Cloonar/coding-lab

Fork 0

perf(tracker): tail the PR-history walks — per-branch done-signal, windowed listings #179

Merged

dominik.polakovics merged 2 commits from afk/176 into main

2026-07-20 20:29:13 +02:00

dominik.polakovics commented

2026-07-19 00:47:27 +02:00

Owner

Problem (#176)

Everything touching the forge slowed as PR history grew, forever. Tracker.Pulls() listed every PR the repo ever had — state=all, 50/page, sequentially until an empty page — and two hot paths ran that walk: the AFK reaper once per repo per tick (854 walks in 17 h, only to ask "does an open-or-merged PR exist for this run's branch"), and labctl pr list (6.7 s measured while the reaper's own request stream loaded the forge). The Issues(closed|all) views had the same unbounded shape.

Change

Done-signal → PullsForHead(head, base) — a new seam method on all three backends, one bounded read per active run per tick (O(active runs), independent of history), with its own pulls_for_head metrics op:

Forgejo: GET /pulls/{base}/{head} fast path (branch names path-escaped — afk/N travels as one segment). Two live-verified quirks pinned in tests: a merged PR whose branch was deleted renders a synthetic refs/pull/N/head, so HeadBranch is filled from the queried name; and the upstream lookup has no ORDER BY, so only a closed-unmerged answer (which could shadow a live sibling and falsely fail a run) pays for a fallback walk — 404/open/merged stay one request.
GitHub: server-side head=owner:branch filter (owner prefix mandatory) + defensive client-side re-filter.
Builtin: in-memory filter over the one cheap store query.

Semantics preserved exactly: DonePull/PullState remain the single collision authority (open > merged > closed; closed-unmerged is not a done-signal), conformance-tested with the same table across all three backends. The read is base-scoped (the base handlePRCreate pins for every lab PR); the reaper gains per-run error isolation.

Pulls() and Issues(closed|all) → windowed: full open set + the RecentClosedWindow (50) most recently closed items (sort=recentclose/recentupdate on Forgejo, updated-desc on GitHub, number DESC LIMIT on the builtin store). The walk-to-empty-page pattern (and its guaranteed wasted probe) survives only where the open working set bounds it. Request counts are pinned by tests. labctl pr list documents the window in its usage line.

Docs: ADR-0048 records the decisions; ADR-0009's "Pulls are fetched in ALL states" pin gains a revision pointer; CONTEXT.md's done-signal relationship updated.

Acceptance criteria from the brief

Reaper tick issues O(active runs) tracker requests (test: 3 runs ⇒ exactly 3 PullsForHead calls, zero Pulls calls)
Done-signal conformance-tested across forgejo/github/builtin: open counts, merged counts, closed-unmerged does not
pr list request count constant in history (test pins: forgejo 60-open repo ⇒ exactly 4 requests however deep the closed history)
pr list still shows all open PRs + recently merged window (documented in usage)
New seam method instrumented (pulls_for_head)
Sibling Issues(state) walk bounded the same way (closed/all views)

Verification

Full suite + golangci-lint (0 issues) green; internal/tmuxx integration failures are environmental (identical on clean HEAD in this sandbox — no tmux session spawn).
Live against git.cloonar.com (80 PRs today): deployed full walk = 3 requests / ~1.7 s unloaded (6.7 s under incident load), +1 page per 50 merges; bounded pr list pattern = 2 requests / ~1.1 s constant; per-run done-signal fast path = 1 request / ~70 ms. Post-deploy, lab_tracker_requests_total{op="pulls"} should stop growing per tick in favor of op="pulls_for_head".

Closes #176

🤖 Generated with Claude Code

## Problem (#176) Everything touching the forge slowed as PR history grew, forever. `Tracker.Pulls()` listed every PR the repo ever had — `state=all`, 50/page, sequentially until an empty page — and two hot paths ran that walk: the AFK reaper once per repo per tick (854 walks in 17 h, only to ask "does an open-or-merged PR exist for this run's branch"), and `labctl pr list` (6.7 s measured while the reaper's own request stream loaded the forge). The `Issues(closed|all)` views had the same unbounded shape. ## Change **Done-signal → `PullsForHead(head, base)`** — a new seam method on all three backends, one bounded read per active run per tick (O(active runs), independent of history), with its own `pulls_for_head` metrics op: - **Forgejo**: `GET /pulls/{base}/{head}` fast path (branch names path-escaped — `afk/N` travels as one segment). Two live-verified quirks pinned in tests: a merged PR whose branch was deleted renders a synthetic `refs/pull/N/head`, so `HeadBranch` is filled from the queried name; and the upstream lookup has no ORDER BY, so only a closed-unmerged answer (which could shadow a live sibling and falsely fail a run) pays for a fallback walk — 404/open/merged stay one request. - **GitHub**: server-side `head=owner:branch` filter (owner prefix mandatory) + defensive client-side re-filter. - **Builtin**: in-memory filter over the one cheap store query. Semantics preserved exactly: `DonePull`/`PullState` remain the single collision authority (open > merged > closed; closed-unmerged is not a done-signal), conformance-tested with the same table across all three backends. The read is base-scoped (the base `handlePRCreate` pins for every lab PR); the reaper gains per-run error isolation. **`Pulls()` and `Issues(closed|all)` → windowed**: full open set + the `RecentClosedWindow` (50) most recently closed items (`sort=recentclose`/`recentupdate` on Forgejo, `updated`-desc on GitHub, `number DESC LIMIT` on the builtin store). The walk-to-empty-page pattern (and its guaranteed wasted probe) survives only where the open working set bounds it. Request counts are pinned by tests. `labctl pr list` documents the window in its usage line. **Docs**: ADR-0048 records the decisions; ADR-0009's "Pulls are fetched in ALL states" pin gains a revision pointer; CONTEXT.md's done-signal relationship updated. ## Acceptance criteria from the brief - [x] Reaper tick issues O(active runs) tracker requests (test: 3 runs ⇒ exactly 3 `PullsForHead` calls, zero `Pulls` calls) - [x] Done-signal conformance-tested across forgejo/github/builtin: open counts, merged counts, closed-unmerged does not - [x] `pr list` request count constant in history (test pins: forgejo 60-open repo ⇒ exactly 4 requests however deep the closed history) - [x] `pr list` still shows all open PRs + recently merged window (documented in usage) - [x] New seam method instrumented (`pulls_for_head`) - [x] Sibling `Issues(state)` walk bounded the same way (closed/all views) ## Verification - Full suite + `golangci-lint` (0 issues) green; `internal/tmuxx` integration failures are environmental (identical on clean HEAD in this sandbox — no tmux session spawn). - Live against git.cloonar.com (80 PRs today): deployed full walk = 3 requests / ~1.7 s unloaded (6.7 s under incident load), +1 page per 50 merges; bounded `pr list` pattern = 2 requests / ~1.1 s constant; per-run done-signal fast path = 1 request / ~70 ms. Post-deploy, `lab_tracker_requests_total{op="pulls"}` should stop growing per tick in favor of `op="pulls_for_head"`. Closes #176 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dominik.polakovics added 1 commit

2026-07-19 00:47:27 +02:00

perf(tracker): bounded per-branch done-signal and windowed listings

ci / native (pull_request) Successful in 6m20s

Details

cdb939af44

Issue #176: every forge read grew with repo history, forever. Pulls()
walked the full PR history (state=all, 50/page, until an empty page) and
two hot paths ran it: the AFK reaper once per repo per tick (854 walks
in 17h, ~1.3s/page, one page added per 50 merged PRs) and labctl pr list
(6.7s measured while the reaper's own request stream loaded the forge).
The Issues closed/all views had the same shape.

- New seam method PullsForHead(head, base): the reaper's done-signal is
  one bounded read per active run per tick, O(active runs) independent
  of history. Forgejo rides GET /pulls/{base}/{head} (branch names
  path-escaped; HeadBranch filled from the queried name because the
  forge renders deleted-branch heads as synthetic refs/pull/N/head),
  with a fallback walk only on a closed-unmerged answer: the upstream
  by-base-head lookup has no ORDER BY, so a closed row may shadow a
  live sibling — the one direction that would falsely fail a run.
  GitHub filters server-side (head=owner:branch, owner prefix
  mandatory); builtin filters its one cheap store query. DonePull and
  PullState stay the single collision authority; new instrumentation
  op pulls_for_head; done-signal semantics (open counts, merged
  counts, closed-unmerged does not) conformance-tested across all
  three backends.

- Pulls() and the Issues closed/all views become bounded: the full
  open set plus the RecentClosedWindow (50) most recently closed
  items — sort=recentclose / recentupdate on Forgejo, updated-desc on
  GitHub, number-desc LIMIT on the builtin store. Request counts are
  pinned by tests and no longer grow with merged history; labctl pr
  list documents the window in its usage line.

- ADR-0048 records the decisions; ADR-0009's "Pulls are fetched in
  ALL states" pin gains the revision pointer; CONTEXT.md's
  done-signal relationship updated.

Live before/after on this repo (80 PRs): the deployed full walk is 3
requests / ~1.7s unloaded (6.7s under incident load) and grows a page
per 50 merges; the bounded pr list pattern is 2 requests / ~1.1s
constant; the reaper's per-run fast path is one request (~70ms).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dominik.polakovics referenced this pull request

2026-07-20 16:17:34 +02:00

autoland: one priority-ordered spawn loop — drain the pipeline before filling it #185

dominik.polakovics commented

2026-07-20 20:09:58 +02:00

Author

Owner

[autoland] verdict: reject

FAIL — the PR is sound on its own merits, but it no longer merges into main, and the resolution is semantic, not mechanical. An autonomous lander must not pick a side on that.

Verification signal relied on: labctl pr checks 179 --wait → aggregate success (ci / native (pull_request), 6m20s, run 200). That green vouches for the branch tip as authored — it does not vouch for the merged tree, and the merged tree is where the problem is.

What passes

Title perf(tracker): tail the PR-history walks — per-branch done-signal, windowed listings — Conventional Commits ✓
Body carries Closes #176 ✓
Diff scope matches #176's brief (seam method on all three backends, conformance tests, windowed Issues/Pulls, pulls_for_head op, ADR + CONTEXT.md) — no drive-by changes ✓

Blockers

main advanced by four merged PRs (#183, #184, #186, #187, #190) that rework the exact reaper path and tracker seam this PR touches. git merge origin/main on the head branch conflicts:

1. internal/afk/reaper.go — semantic, both sides rewrote the same loop. This PR replaces the per-repo trk.Pulls(ctx) listing with a per-run trk.PullsForHead(ctx, run.Branch, repo.DefaultBranch). main (#182/#181) kept the single listing and added verdict kinds on top of it:

donePull, prPresent := tracker.DonePull(pulls, run.Branch)
done, escalated := prPresent, false
switch run.Kind {
case store.RunKindLander, store.RunKindFix, store.RunKindEscalate:
    done, escalated, readable = s.verdictDoneSignal(ctx, trk, repo, run, donePull, prPresent)
    ...
}
outcome, alive, claimed := s.classifyAndClaim(ctx, run, done, escalated, now)

Resolving this is a design decision, not a merge: the bounded per-branch read has to be shown correct for the verdict kinds (whose PR pre-exists the run), classifyAndClaim's new (done, escalated) signature has to meet this PR's per-run error isolation, and the two continue-on-unreadable paths have to compose. Escalating rather than guessing.

2. internal/afk/engine_test.go — fakeTracker conflict (2 hunks). The fake must now serve both this PR's PullsForHead counters/args and main's Reviews/PullComments autoland reads. Falls out of (1); the pullsCalls must-stay-zero assertion needs rechecking against the verdict-kind path.

3. internal/tracker/instrument_test.go — opOrder. Mechanical (union OpPullsForHead into main's list, which added OpReviews, OpRerequestReview, OpCommentPull, OpPullComments) — but it must match driveAll's order, so it lands with (1).

4. ADR number collision — not a git conflict, so the merge will not catch it. This PR adds docs/adr/0048-bounded-tracker-reads.md; main already ships docs/adr/0048-autoland-fix-forward.md and 0049-one-priority-ordered-spawn-pass.md. Renumber to 0050 and update the back-reference added in docs/adr/0009-tracker-seam-builtin-issues.md plus any CONTEXT.md pointer.

To hand back

Merge origin/main into afk/176 (merge, never rebase — the PR is open), resolve (1)–(3) with the verdict-kind interaction settled explicitly, renumber the ADR per (4), then re-run the full suite + golangci-lint on the resolved tree — CI's current green predates all of this — and labctl pr rerequest 179.

[autoland] verdict: reject **FAIL** — the PR is sound on its own merits, but it no longer merges into `main`, and the resolution is semantic, not mechanical. An autonomous lander must not pick a side on that. **Verification signal relied on:** `labctl pr checks 179 --wait` → aggregate `success` (`ci / native (pull_request)`, 6m20s, run 200). That green vouches for the branch tip as authored — it does **not** vouch for the merged tree, and the merged tree is where the problem is. ## What passes - Title `perf(tracker): tail the PR-history walks — per-branch done-signal, windowed listings` — Conventional Commits ✓ - Body carries `Closes #176` ✓ - Diff scope matches #176's brief (seam method on all three backends, conformance tests, windowed `Issues`/`Pulls`, `pulls_for_head` op, ADR + CONTEXT.md) — no drive-by changes ✓ ## Blockers `main` advanced by four merged PRs (#183, #184, #186, #187, #190) that rework the exact reaper path and tracker seam this PR touches. `git merge origin/main` on the head branch conflicts: **1. `internal/afk/reaper.go` — semantic, both sides rewrote the same loop.** This PR replaces the per-repo `trk.Pulls(ctx)` listing with a per-run `trk.PullsForHead(ctx, run.Branch, repo.DefaultBranch)`. `main` (#182/#181) kept the single listing and added verdict kinds on top of it: ```go donePull, prPresent := tracker.DonePull(pulls, run.Branch) done, escalated := prPresent, false switch run.Kind { case store.RunKindLander, store.RunKindFix, store.RunKindEscalate: done, escalated, readable = s.verdictDoneSignal(ctx, trk, repo, run, donePull, prPresent) ... } outcome, alive, claimed := s.classifyAndClaim(ctx, run, done, escalated, now) ``` Resolving this is a design decision, not a merge: the bounded per-branch read has to be shown correct for the verdict kinds (whose PR *pre-exists* the run), `classifyAndClaim`'s new `(done, escalated)` signature has to meet this PR's per-run error isolation, and the two `continue`-on-unreadable paths have to compose. Escalating rather than guessing. **2. `internal/afk/engine_test.go` — `fakeTracker` conflict (2 hunks).** The fake must now serve *both* this PR's `PullsForHead` counters/args and `main`'s `Reviews`/`PullComments` autoland reads. Falls out of (1); the `pullsCalls` must-stay-zero assertion needs rechecking against the verdict-kind path. **3. `internal/tracker/instrument_test.go` — `opOrder`.** Mechanical (union `OpPullsForHead` into `main`'s list, which added `OpReviews`, `OpRerequestReview`, `OpCommentPull`, `OpPullComments`) — but it must match `driveAll`'s order, so it lands with (1). **4. ADR number collision — not a git conflict, so the merge will not catch it.** This PR adds `docs/adr/0048-bounded-tracker-reads.md`; `main` already ships `docs/adr/0048-autoland-fix-forward.md` and `0049-one-priority-ordered-spawn-pass.md`. Renumber to **0050** and update the back-reference added in `docs/adr/0009-tracker-seam-builtin-issues.md` plus any `CONTEXT.md` pointer. ## To hand back Merge `origin/main` into `afk/176` (merge, never rebase — the PR is open), resolve (1)–(3) with the verdict-kind interaction settled explicitly, renumber the ADR per (4), then re-run the full suite + `golangci-lint` on the *resolved* tree — CI's current green predates all of this — and `labctl pr rerequest 179`.

dominik.polakovics added 1 commit

2026-07-20 20:20:53 +02:00

Merge branch 'main' into afk/176

ci / native (pull_request) Successful in 6m54s

Details

c1c5fc7e4c

Resolves the reaper done-signal seam against the autoland verdict kinds
(#181/#182 landed on main after this branch forked):

- reaper.go: the bounded per-run PullsForHead read (#176) now feeds
  main's kind-derived done-signal — DonePull projects the winner from
  the per-branch candidates, then the verdict kinds derive done from
  verdictDoneSignal exactly as on main. Sound because a verdict-kind
  PR was created by the same agentapi handlePRCreate (head = the run's
  branch, base = the repo default branch) and its head branch outlives
  the run (MergePull never deletes it), so the seam's LIVE-branch
  contract holds for every kind. The two skip-on-missing-data paths
  (failed PullsForHead read, unreadable comments) are both per-run and
  compose.
- engine_test.go: fakeTracker is the union — PullsForHead counters/args
  (#176) plus Reviews/PullComments scripting (#181); main's now-unused
  pullsErr/failPulls dropped (every reaper test scripts
  failPullsForHead).
- instrument_test.go: opOrder is the union in driveAll's order.
- ADR renumbered 0048 → 0050 (main shipped 0048-autoland-fix-forward
  and 0049): file moved, ADR-0009 back-reference updated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>