Public: anyone can read this; the crew can change it.
/apps and the dashboard query once per row again: two N+1 guards are red #503
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/292-apps-and-the-dashboard-query-once-per-ro → main 5 commits fork
7 files
changed +120
−15
forge.json +1 −1
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | "ready": 90 |
| 7 | 7 | }, |
| 8 | 8 | "check": { |
| 9 | "run": "bun x tsc --noEmit && bun scripts/test-shards.ts --timeout 60000 -- crew crew-wrapup crew-limits crew-parks crew-sandbox steer session-settle landing decide ledger checks dependencies autonomy studio studio-words forks fork-flow cross-repo-pull pulls apps apps-adversarial apps-busy apps-token web git-trust helpers shards migrate vitals guard asks feed feed-page offline share cloud run-secrets forge-cloudflare usage credentials standing board-windows spawn-plan spawn web-apps fleet-ops meter told-by", | |
| 9 | "run": "bun x tsc --noEmit && bun scripts/test-shards.ts --timeout 60000 -- crew crew-wrapup crew-limits crew-parks crew-sandbox steer session-settle landing decide ledger checks dependencies autonomy studio studio-words forks fork-flow cross-repo-pull pulls apps apps-adversarial apps-busy apps-token surfaces-adversarial orgs web git-trust helpers shards migrate vitals guard asks feed feed-page offline share cloud run-secrets forge-cloudflare usage credentials standing board-windows spawn-plan spawn web-apps fleet-ops meter told-by", | |
| 10 | 10 | "timeout": 1800 |
| 11 | 11 | } |
| 12 | 12 | } |
src/core/orgs.ts +25
| @@ -248,6 +248,31 @@ export function orgAccess( | ||
| 248 | 248 | return best; |
| 249 | 249 | } |
| 250 | 250 | |
| 251 | /** | |
| 252 | * `orgAccess` for every org the user is on a team in, in one query: the | |
| 253 | * lists that must ask "may this viewer write here" of many org-owned repos | |
| 254 | * at once read this map instead of calling `orgAccess` per row. Site admins | |
| 255 | * are the caller's to short-circuit; an org missing from the map is "none". | |
| 256 | */ | |
| 257 | export function orgAccessByOrg( | |
| 258 | db: Database, | |
| 259 | user: User | undefined, | |
| 260 | ): Map<string, Access> { | |
| 261 | const out = new Map<string, Access>(); | |
| 262 | if (!user) return out; | |
| 263 | const rows = db | |
| 264 | .query<{ org_id: string; permission: Permission }, [string]>( | |
| 265 | `SELECT team.org_id, team.permission FROM team | |
| 266 | JOIN team_member ON team_member.team_id = team.id | |
| 267 | WHERE team_member.user_id = ?1`, | |
| 268 | ) | |
| 269 | .all(user.id); | |
| 270 | for (const row of rows) | |
| 271 | if (rank[row.permission] > rank[out.get(row.org_id) ?? "none"]) | |
| 272 | out.set(row.org_id, row.permission); | |
| 273 | return out; | |
| 274 | } | |
| 275 | ||
| 251 | 276 | export function isOrgMember( |
| 252 | 277 | db: Database, |
| 253 | 278 | orgId: string, |
src/core/repos.ts +32 −1
| @@ -21,7 +21,12 @@ import { createLog } from "../kernel/log.ts"; | ||
| 21 | 21 | import { err, ok, type Result } from "../kernel/result.ts"; |
| 22 | 22 | import { likeEscape, pageLimit } from "./issues.ts"; |
| 23 | 23 | import { REPO_VISIBLE, REPO_WRITABLE } from "./visibility.ts"; |
| 24 | import { accessAllows, orgAccess, type Access } from "./orgs.ts"; | |
| 24 | import { | |
| 25 | accessAllows, | |
| 26 | orgAccess, | |
| 27 | orgAccessByOrg, | |
| 28 | type Access, | |
| 29 | } from "./orgs.ts"; | |
| 25 | 30 | import { getUserById, type User } from "./users.ts"; |
| 26 | 31 | |
| 27 | 32 | const log = createLog("repos"); |
| @@ -661,6 +666,32 @@ export function canSeeOwnerFacts( | ||
| 661 | 666 | return canWrite(db, repo, user); |
| 662 | 667 | } |
| 663 | 668 | |
| 669 | /** | |
| 670 | * `canSeeOwnerFacts` for many repositories at once: the ids of those whose | |
| 671 | * owner facts the user may see. The same rules as `canWrite` — owner, site | |
| 672 | * admin, or a write/admin team in the owning org (no crew grant, unlike | |
| 673 | * `REPO_WRITABLE`) — answered from one read of the user's teams, so a list | |
| 674 | * does not pay a user lookup and a permission query per row. | |
| 675 | */ | |
| 676 | export function ownerFactRepoIds( | |
| 677 | db: Database, | |
| 678 | repos: readonly Repo[], | |
| 679 | user: User | undefined, | |
| 680 | ): Set<string> { | |
| 681 | const out = new Set<string>(); | |
| 682 | if (user === undefined) return out; | |
| 683 | const orgs = user.isAdmin | |
| 684 | ? new Map<string, Access>() | |
| 685 | : orgAccessByOrg(db, user); | |
| 686 | for (const repo of repos) | |
| 687 | if ( | |
| 688 | personalOrDirect(repo, user) || | |
| 689 | accessAllows(orgs.get(repo.ownerId) ?? "none", "write") | |
| 690 | ) | |
| 691 | out.add(repo.id); | |
| 692 | return out; | |
| 693 | } | |
| 694 | ||
| 664 | 695 | /** |
| 665 | 696 | * Which of these repositories the user may write — `canWrite` for many rows |
| 666 | 697 | * in one query per chunk, through `REPO_WRITABLE`. The dashboard's inbox |
src/web/pages/board.ts +2 −6
| @@ -37,8 +37,8 @@ import { getIssue, getIssuesByIds, type Issue } from "../../core/issues.ts"; | ||
| 37 | 37 | import { looksForApps } from "../../core/looks.ts"; |
| 38 | 38 | import { listPulls, mergedPullsForRepos, toldBy, type Pull } from "../../core/pulls.ts"; |
| 39 | 39 | import { |
| 40 | canSeeOwnerFacts, | |
| 41 | 40 | listReposVisibleTo, |
| 41 | ownerFactRepoIds, | |
| 42 | 42 | type Repo, |
| 43 | 43 | } from "../../core/repos.ts"; |
| 44 | 44 | import { listActiveRuns, listRuns, type Run } from "../../core/runs.ts"; |
| @@ -124,11 +124,7 @@ function boardScope( | ||
| 124 | 124 | ids: kept.map((r) => r.id), |
| 125 | 125 | byId: new Map(kept.map((r) => [r.id, r])), |
| 126 | 126 | records, |
| 127 | ownerFacts: new Set( | |
| 128 | kept | |
| 129 | .filter((r) => canSeeOwnerFacts(ctx.app.db, r, ctx.auth.user)) | |
| 130 | .map((r) => r.id), | |
| 131 | ), | |
| 127 | ownerFacts: ownerFactRepoIds(ctx.app.db, kept, ctx.auth.user), | |
| 132 | 128 | }; |
| 133 | 129 | } |
| 134 | 130 |
src/web/studio/asks.ts +3 −2
| @@ -22,9 +22,9 @@ import { | ||
| 22 | 22 | import { looksForApps } from "../../core/looks.ts"; |
| 23 | 23 | import { joinMany, type Pull } from "../../core/pulls.ts"; |
| 24 | 24 | import { |
| 25 | canSeeOwnerFacts, | |
| 26 | 25 | listReposVisibleTo, |
| 27 | 26 | listReposWritableTo, |
| 27 | ownerFactRepoIds, | |
| 28 | 28 | type Repo, |
| 29 | 29 | } from "../../core/repos.ts"; |
| 30 | 30 | import { listReviews } from "../../core/reviews.ts"; |
| @@ -159,7 +159,8 @@ function askCandidatesFor( | ||
| 159 | 159 | // Shipping, closing and the day's ceiling are the owner's own decisions — |
| 160 | 160 | // a reader who cannot write here is never asked to make one, and never |
| 161 | 161 | // sees the dollar figure a "paused" card would otherwise carry. |
| 162 | const repos = allRepos.filter((r) => canSeeOwnerFacts(db, r, user)); | |
| 162 | const ownerFacts = ownerFactRepoIds(db, allRepos, user); | |
| 163 | const repos = allRepos.filter((r) => ownerFacts.has(r.id)); | |
| 163 | 164 | const byId = new Map(repos.map((r) => [r.id, r])); |
| 164 | 165 | const pulls = listOpenPullsVisibleTo(db, user, 50).filter((p) => |
| 165 | 166 | byId.has(p.repoId), |
src/web/studio/shelf.ts +2 −4
| @@ -29,7 +29,7 @@ import { appsForRepos } from "../../core/apps.ts"; | ||
| 29 | 29 | import { reposWithBuilder } from "../../core/agents.ts"; |
| 30 | 30 | import { reposWithUnclaimedTell } from "../../core/issues.ts"; |
| 31 | 31 | import { openPullsForRepos } from "../../core/pulls.ts"; |
| 32 | import { canSeeOwnerFacts, type Repo } from "../../core/repos.ts"; | |
| 32 | import { ownerFactRepoIds, type Repo } from "../../core/repos.ts"; | |
| 33 | 33 | import { |
| 34 | 34 | getRunsByIds, |
| 35 | 35 | isTerminal, |
| @@ -119,9 +119,7 @@ export async function shelfRows( | ||
| 119 | 119 | // A public app you can only read appears here too, but who is on its |
| 120 | 120 | // crew is its owner's fact, not yours to see — same predicate as the |
| 121 | 121 | // app's own page. |
| 122 | const ownerFacts = new Set( | |
| 123 | repos.filter((r) => canSeeOwnerFacts(db, r, user)).map((r) => r.id), | |
| 124 | ); | |
| 122 | const ownerFacts = ownerFactRepoIds(db, repos, user); | |
| 125 | 123 | // One read for the newest run of every app, one for who ran them: the |
| 126 | 124 | // shelf must not get slower as the apps grow. |
| 127 | 125 | const latestRuns = latestAgentRunsForRepos( |
test/orgs.test.ts +55 −1
| @@ -14,7 +14,13 @@ import { | ||
| 14 | 14 | removeTeamMember, |
| 15 | 15 | OWNERS_TEAM, |
| 16 | 16 | } from "../src/core/orgs.ts"; |
| 17 | import { canRead, canWrite, canAdmin } from "../src/core/repos.ts"; | |
| 17 | import { | |
| 18 | canRead, | |
| 19 | canSeeOwnerFacts, | |
| 20 | canWrite, | |
| 21 | canAdmin, | |
| 22 | ownerFactRepoIds, | |
| 23 | } from "../src/core/repos.ts"; | |
| 18 | 24 | import { createRepo, getRepo } from "../src/core/repos.ts"; |
| 19 | 25 | import { createUser, getUserByName } from "../src/core/users.ts"; |
| 20 | 26 | import { serve, type Servers } from "../src/http/server.ts"; |
| @@ -119,6 +125,54 @@ describe("org & team core", () => { | ||
| 119 | 125 | expect(canAdmin(app.db, repo.value, dev)).toBe(false); |
| 120 | 126 | }); |
| 121 | 127 | |
| 128 | test("ownerFactRepoIds answers canSeeOwnerFacts for many repos in one query", async () => { | |
| 129 | const founder = getUserByName(app.db, "founder")!; | |
| 130 | const dev = getUserByName(app.db, "dev")!; | |
| 131 | const reader = getUserByName(app.db, "reader")!; | |
| 132 | const outsider = getUserByName(app.db, "outsider")!; | |
| 133 | const org = getOrg(app.db, "acme")!; | |
| 134 | const made = await Promise.all([ | |
| 135 | createRepo(app.db, app.reposRoot, { | |
| 136 | ownerId: org.id, | |
| 137 | name: "facts-public", | |
| 138 | visibility: "public", | |
| 139 | }), | |
| 140 | createRepo(app.db, app.reposRoot, { | |
| 141 | ownerId: dev.id, | |
| 142 | name: "facts-own", | |
| 143 | visibility: "private", | |
| 144 | }), | |
| 145 | createRepo(app.db, app.reposRoot, { | |
| 146 | ownerId: outsider.id, | |
| 147 | name: "facts-other", | |
| 148 | visibility: "public", | |
| 149 | }), | |
| 150 | ]); | |
| 151 | const repos = [getRepo(app.db, "acme", "svc")!]; | |
| 152 | for (const r of made) if (r.ok) repos.push(r.value); | |
| 153 | expect(repos.length).toBe(4); | |
| 154 | ||
| 155 | for (const user of [founder, dev, reader, outsider, undefined]) { | |
| 156 | let queries = 0; | |
| 157 | const original = app.db.query.bind(app.db); | |
| 158 | (app.db as unknown as { query: unknown }).query = (sql: string) => { | |
| 159 | queries++; | |
| 160 | return original(sql); | |
| 161 | }; | |
| 162 | let batched: Set<string>; | |
| 163 | try { | |
| 164 | batched = ownerFactRepoIds(app.db, repos, user); | |
| 165 | } finally { | |
| 166 | (app.db as unknown as { query: unknown }).query = original; | |
| 167 | } | |
| 168 | expect(queries).toBeLessThanOrEqual(1); | |
| 169 | const oneByOne = repos | |
| 170 | .filter((r) => canSeeOwnerFacts(app.db, r, user)) | |
| 171 | .map((r) => r.id); | |
| 172 | expect([...batched].sort()).toEqual(oneByOne.sort()); | |
| 173 | } | |
| 174 | }); | |
| 175 | ||
| 122 | 176 | test("the Owners team can't be emptied", async () => { |
| 123 | 177 | const org = getOrg(app.db, "acme")!; |
| 124 | 178 | const owners = listTeams(app.db, org.id).find( |
new above
offline