From 8f60cb3161aa6d28efba430fe6d47f37cf42047d Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Wed, 28 Oct 2026 17:17:58 +0000 Subject: [PATCH 1/2] stewards: refresh project configs off the read path and count picker needs in SQL (BB-104) --- plugins/stewards/server.test.ts | 173 ++++++++++++++++++++++++++++---- plugins/stewards/server.ts | 59 ++++++++--- plugins/stewards/store.test.ts | 51 +++++++++- plugins/stewards/store.ts | 20 ++++ 4 files changed, 266 insertions(+), 37 deletions(-) diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 17ed7800..d4f6fd72 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -455,36 +455,25 @@ describe("a Linear ticket steward", () => { }); it("retries a lost adoption wake: a binding left with no thread is woken as adopted on the next reconcile (BB-92)", async () => { - const t = await setup({ timing: { debounceMs: 1, nightlyStaggerMs: 0, configTtlMs: 0 } }); - // Let the startup sweep finish before counting config reads, so the only - // reads are the ring's adoption and its first reconcile. + const t = await setup(); + // Let the startup sweep finish, then start from the state a restart leaves: + // the binding exists with no thread, and no configuration cached. await t.flush(); t.entities.set("uuid-12", issue()); - // The first reconcile after adoption reads the config as missing (the - // transient read behind BB-90): adoption still creates the binding, but - // the forced wake exits before the steward thread is spawned. - let projectFile = t.files[PROJECT_FILE]; - let reads = 0; - Object.defineProperty(t.files, PROJECT_FILE, { - configurable: true, - get() { - reads += 1; - // Read 1 is the ring's stewardedProjects; read 2 is the forced - // adoption reconcile, which must find nothing and exit silently. - return reads === 2 ? null : projectFile; - }, - set(value: string | null) { - projectFile = value; - }, - }); + t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-12", entityKey: "ENG-12", url: null, title: "Fix login", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "Todo", stage: "spec" }); + t.hooks.forgetConfigs(); + const projectFile = t.files[PROJECT_FILE]; + t.files[PROJECT_FILE] = null; + // The cold read behind BB-90 finds no configuration, so the reconcile + // exits before the steward thread is spawned, leaving it for the next. await t.ring("linear", "uuid-12"); - // Adopted, but the wake was lost: no thread at all (the project steward is - // spawned lazily with the first steward), and no wake recorded. expect(t.spawns).toEqual([]); expect(await t.list()).toMatchObject([{ key: "ENG-12", threadId: null }]); expect(t.hooks.store.find("ENG-12", "p1")!.wokeAt).toBeNull(); // A plain reconcile (force null) retries the adoption wake on its own. + t.files[PROJECT_FILE] = projectFile; + t.hooks.forgetConfigs(); await t.ring("linear", "uuid-12"); expect(await t.list()).toMatchObject([{ key: "ENG-12", threadId: t.spawns[1]!.id }]); expect(t.spawns[1]).toMatchObject({ parentThreadId: t.spawns[0]!.id, parentNotifications: "off" }); @@ -3531,6 +3520,127 @@ describe("questions to the user", () => { }); }); +// BB-104: the project configuration is served stale and refreshed in the +// background, so the first poll after the TTL never blocks on the multi-repo +// read, and a burst of polls makes one read. +describe("stale-while-revalidate project configuration (BB-104)", () => { + type LoadConfig = (projectId: string, force?: boolean) => Promise<{ loadedAt: number } | null>; + + /** Hold every `readProjectFiles` open until released, and count them. */ + const gated = (t: Awaited>) => { + const original = t.sdk.plugins.callRpc; + const reads: string[][] = []; + let gate: Promise | null = null; + let release: () => void = () => {}; + t.harness.sdk.stub("plugins.callRpc", async (args: { pluginId: string; method: string; input?: unknown; outputSchema: { parse(v: unknown): unknown } }) => { + if (args.method === "readProjectFiles") { + reads.push((args.input as { paths: string[] }).paths); + if (gate !== null) await gate; + } + return original(args); + }); + return { + reads, + hold() { + gate = new Promise((resolve) => { + release = resolve; + }); + }, + release() { + release(); + gate = null; + }, + }; + }; + + const load = (t: Awaited>, force?: boolean) => (t.hooks.loadConfig as LoadConfig)("p1", force); + + it("returns the stale entry at once, refreshes it in the background, and replaces it afterwards", async () => { + const t = await setup(); + await t.flush(); + const stale = await load(t); + expect(stale).not.toBeNull(); + const gate = gated(t); + gate.hold(); + t.advance(10 * 60_000); + + // The sidebar does not wait for the refresh. + const result = (await t.harness.callRpc("stewardsSidebar", { projectId: "p1" })) as { projectId: string | null }; + expect(result.projectId).toBe("p1"); + expect(gate.reads.filter((paths) => paths.includes(PROJECT_FILE))).toHaveLength(1); + + // The background read replaces the entry. + gate.release(); + await t.flush(); + const refreshed = await load(t); + expect(refreshed!.loadedAt).toBe(t.now()); + expect(refreshed!.loadedAt).not.toBe(stale!.loadedAt); + }); + + it("shares one in-flight read between concurrent stale calls", async () => { + const t = await setup(); + await t.flush(); + const gate = gated(t); + gate.hold(); + t.advance(10 * 60_000); + const first = load(t); + const second = load(t); + await Promise.resolve(); + expect(gate.reads.filter((paths) => paths.includes(PROJECT_FILE))).toHaveLength(1); + gate.release(); + await Promise.all([first, second]); + }); + + it("keeps the last good entry and logs when the refresh fails", async () => { + const t = await setup(); + await t.flush(); + const before = await load(t); + const original = t.sdk.plugins.callRpc; + t.harness.sdk.stub("plugins.callRpc", async (args: { pluginId: string; method: string; input?: unknown; outputSchema: { parse(v: unknown): unknown } }) => { + if (args.method === "readProjectFiles") throw new Error("the .bb is unreachable"); + return original(args); + }); + t.advance(10 * 60_000); + expect((await load(t))!.loadedAt).toBe(before!.loadedAt); + await t.flush(); + expect(t.harness.logEntries.some((entry) => entry.level === "warn" && entry.message.includes("could not read the configuration of project p1"))).toBe(true); + // The old entry is still served, and the next call refreshes again. + expect((await load(t))!.loadedAt).toBe(before!.loadedAt); + }); + + it("still awaits the read for a forced call and for a cold cache", async () => { + const t = await setup(); + await t.flush(); + // force: the TTL does not matter, the caller waits for the read. + const forcedGate = gated(t); + forcedGate.hold(); + t.advance(10 * 60_000); + let forcedSettled = false; + const forced = load(t, true).then((v) => { + forcedSettled = true; + return v; + }); + await Promise.resolve(); + expect(forcedSettled).toBe(false); + forcedGate.release(); + expect((await forced)!.loadedAt).toBe(t.now()); + + // cold: no entry, so the call blocks on the read as it always did. + t.hooks.forgetConfigs(); + const coldGate = gated(t); + coldGate.hold(); + let coldSettled = false; + const cold = load(t).then((v) => { + coldSettled = true; + return v; + }); + await Promise.resolve(); + expect(coldSettled).toBe(false); + coldGate.release(); + expect(await cold).not.toBeNull(); + }); +}); + describe("the stewards sidebar call", () => { type Sidebar = { projects: Array<{ projectId: string; name: string; needsCount: number }>; @@ -3581,6 +3691,25 @@ describe("the stewards sidebar call", () => { expect(result.ended).toEqual([]); }); + it("counts the picker's needs from the same rows as the yellow ones (BB-104)", async () => { + const t = await setup(); + t.entities.set("uuid-12", issue({ state: "In Progress", stateType: "started" })); + await t.ring("linear", "uuid-12"); + const first = t.hooks.store.find("ENG-12", "p1")!; + // One binding needs the user through its report actor, another through an + // open need; an ended one must not count. + t.hooks.store.patch(first.id, { status: { phase: "In Review", summary: "Waiting", nextAction: "On the user", actor: "user", blockedOn: null, requeueAfterMs: null, conditions: [], resources: { workers: [], pullRequests: [], children: [] }, at: t.now() } }); + const second = t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-13", entityKey: "ENG-13", url: null, title: "Other", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "Todo", stage: "spec" }); + t.hooks.store.fileNeed({ bindingId: second.id, kind: "configure", url: "", what: "Enable it", how: null }); + const ended = t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-14", entityKey: "ENG-14", url: null, title: "Ended", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "Todo", stage: "spec" }); + t.hooks.store.patch(ended.id, { endedAt: t.now() }); + + const result = (await t.harness.callRpc("stewardsSidebar", { projectId: "p1" })) as Sidebar; + const yellow = result.bindings.filter((b) => b.needsUser).length; + expect(yellow).toBe(2); + expect(result.projects.find((p) => p.projectId === "p1")!.needsCount).toBe(yellow); + }); + it("null picks the first project by name, as the board does (BB-100)", async () => { const t = await setup(); const originalProjects = t.sdk.projects.list; diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 0cb9aadb..f52b9b81 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -890,6 +890,11 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // ---- configuration ---------------------------------------------------------------- const configs = new Map(); + /** + * The read in flight for a project, so a burst of calls after the TTL + * makes one read rather than one per caller (BB-104). + */ + const configReads = new Map>(); async function readFiles(projectId: string, paths: string[]): Promise> { const out = await rpc(MULTI_REPO_PLUGIN_ID, "readProjectFiles", { projectId, paths }, readFilesOutput); @@ -897,25 +902,49 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P return new Map(out.files.map((f) => [f.path, f.content])); } + /** + * Read a project's configuration once: on success the result replaces the + * cache entry, on failure the last good entry is kept. A call while a read + * is in flight joins it, so a burst makes one read (BB-104). + */ + function readConfigShared(projectId: string): Promise { + const inFlight = configReads.get(projectId); + if (inFlight !== undefined) return inFlight; + const promise = readConfig(projectId) + .then((value) => { + configs.set(projectId, { at: now(), value }); + return value; + }) + .catch((error: unknown) => { + // Unreachable `.bb` is not "no stewards": keep what we had. + bb.log.warn(`stewards: could not read the configuration of project ${projectId}: ${errorMessage(error)}`); + return configs.get(projectId)?.value ?? null; + }) + .finally(() => { + if (configReads.get(projectId) === promise) configReads.delete(projectId); + }); + configReads.set(projectId, promise); + return promise; + } + /** * The project's stewards configuration, or null when the project has none. * A file that fails to parse falls back to the last good copy, kept in kv, * and the failure is reported as a problem rather than stopping anything. + * + * Stale-while-revalidate (BB-104): with an entry in hand it is returned at + * once, and one background read refreshes it once it is older than the TTL, + * so the `.bb` read never blocks the sidebar. A miss still blocks, so the + * first read after a restart is unchanged, and `force` still awaits a fresh + * copy for the callers that need one (the sweep, the reconcile, nightly). */ async function loadConfig(projectId: string, force = false): Promise { const cached = configs.get(projectId); - if (!force && cached !== undefined && now() - cached.at < timing.configTtlMs) return cached.value; - let value: ProjectConfig | null; - try { - value = await readConfig(projectId); - } catch (error) { - // Unreachable `.bb` is not "no stewards": keep what we had. - bb.log.warn(`stewards: could not read the configuration of project ${projectId}: ${errorMessage(error)}`); - if (cached !== undefined) return cached.value; - return null; + if (!force && cached !== undefined) { + if (now() - cached.at >= timing.configTtlMs) void readConfigShared(projectId); + return cached.value; } - configs.set(projectId, { at: now(), value }); - return value; + return readConfigShared(projectId); } async function readConfig(projectId: string): Promise { @@ -4999,7 +5028,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // the user, so the pill and the yellow rows cannot disagree (BB-100). const picker = projects.map((p) => ({ ...p, - needsCount: store.all(p.projectId).filter((b) => b.endedAt === null && needsUser(b)).length, + // One SQL count per project, so the picker never parses every binding + // and runs three more queries each (BB-104). + needsCount: store.countNeedsUser(p.projectId), })); const picked = projects.find((p) => p.projectId === wanted) ?? (wanted === null ? projects[0] : undefined); if (picked === undefined) { @@ -7560,12 +7591,12 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P kept.clear(); }; await pinSidebarThreadListProvider(bb); - testHooks.set(bb.pluginId, { sweep, requeueTick, unhideOnce, store, db, settings, forgetTags, nightlyPass: (projectId: string, night: string) => loadConfig(projectId, true).then((c) => (c === null ? undefined : nightlyPass(c, night, true))), settle: () => Promise.all([...chains.values()]).then(() => undefined), sentryDoorbell: onSentryDoorbell, sentryLeases }); + testHooks.set(bb.pluginId, { sweep, requeueTick, unhideOnce, store, db, settings, forgetTags, nightlyPass: (projectId: string, night: string) => loadConfig(projectId, true).then((c) => (c === null ? undefined : nightlyPass(c, night, true))), loadConfig: (projectId: string, force = false) => loadConfig(projectId, force), forgetConfigs: () => configs.clear(), settle: () => Promise.all([...chains.values(), ...configReads.values()]).then(() => undefined), sentryDoorbell: onSentryDoorbell, sentryLeases }); }; } /** Test seams, keyed by plugin id. Not part of any contract. */ -export const testHooks = new Map; requeueTick(): Promise; unhideOnce(): Promise; store: Store; db: { prepare(sql: string): { run(...args: unknown[]): unknown } }; settings: { get(): Promise> }; forgetTags(): void; nightlyPass(projectId: string, night: string): Promise; settle(): Promise; sentryDoorbell(id: string, unit: string | null): void; sentryLeases(): Lease[] }>(); +export const testHooks = new Map; requeueTick(): Promise; unhideOnce(): Promise; store: Store; db: { prepare(sql: string): { run(...args: unknown[]): unknown } }; settings: { get(): Promise> }; forgetTags(): void; nightlyPass(projectId: string, night: string): Promise; loadConfig(projectId: string, force?: boolean): Promise; forgetConfigs(): void; settle(): Promise; sentryDoorbell(id: string, unit: string | null): void; sentryLeases(): Lease[] }>(); export { DEFAULT_PLAYBOOKS, STEWARDS_PLUGIN_ID }; diff --git a/plugins/stewards/store.test.ts b/plugins/stewards/store.test.ts index 0f7e55f0..0249f595 100644 --- a/plugins/stewards/store.test.ts +++ b/plugins/stewards/store.test.ts @@ -1,7 +1,7 @@ import Database from "better-sqlite3"; import { describe, expect, it } from "vitest"; import { z } from "zod"; -import { ACTOR_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS, BASE_MIGRATIONS, CHAIN_MIGRATIONS, GUIDANCE_MIGRATIONS, IDLE_MIGRATIONS, LESSON_COUNTER_MIGRATIONS, LESSON_TIER_MIGRATIONS, MIGRATIONS, RATIONALE_MIGRATIONS, VALIDATION_MIGRATIONS, WORKER_CHECK_MIGRATIONS, createStore, lessonScope, statusRecordSchema } from "./store.js"; +import { ACTOR_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS, BASE_MIGRATIONS, CHAIN_MIGRATIONS, GUIDANCE_MIGRATIONS, IDLE_MIGRATIONS, LESSON_COUNTER_MIGRATIONS, LESSON_TIER_MIGRATIONS, MIGRATIONS, RATIONALE_MIGRATIONS, VALIDATION_MIGRATIONS, WORKER_CHECK_MIGRATIONS, createStore, lessonScope, statusRecordSchema, type Binding } from "./store.js"; /** MIGRATIONS without the groups appended after the ones these order tests assert. */ const beforeActorMigrations = MIGRATIONS.slice(0, MIGRATIONS.length - ACTOR_MIGRATIONS.length); @@ -122,6 +122,55 @@ describe("needs you items", () => { }); }); +describe("countNeedsUser (BB-104)", () => { + // The same predicate `needsUser` in server.ts evaluates from the store's own + // reads, so the SQL count and the yellow rows cannot drift. + const needsUser = (s: ReturnType["s"], b: Binding): boolean => + b.status?.actor === "user" || s.openEscalations(b.id).length > 0 || s.openNeeds(b.id).length > 0 || s.openQuestions(b.projectId).some((q) => q.bindingId === b.id); + + const add = (s: ReturnType["s"], n: number) => + s.create({ projectId: "p1", source: "linear", entityId: `uuid-${n}`, entityKey: `ENG-${n}`, url: null, title: null, kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "Todo", stage: "spec" }); + + const userStatus = (at: number) => ({ phase: "Todo", summary: "Waiting", nextAction: "On the user", actor: "user" as const, blockedOn: null, requeueAfterMs: null, conditions: [], resources: { workers: [], pullRequests: [], children: [] }, at }); + + it("counts exactly the open bindings needsUser flags: each condition once, and once when a binding has all four", () => { + const { s, binding } = store(); + // The base binding: actor `user`. + s.patch(binding.id, { status: userStatus(1_000) }); + // One binding per remaining condition. + const escalation = add(s, 13); + s.escalate({ bindingId: escalation.id, kind: "decision", question: "Q", meanwhile: "m", route: "project", recommendation: null }); + const need = add(s, 14); + s.fileNeed({ bindingId: need.id, kind: "configure", url: "", what: "Enable it", how: null }); + const question = add(s, 15); + s.ask({ projectId: "p1", bindingId: question.id, escalationId: null, question: "Which?", context: null }); + // All four on one binding: still one row. + const all = add(s, 16); + s.patch(all.id, { status: userStatus(1_000) }); + s.escalate({ bindingId: all.id, kind: "decision", question: "Q", meanwhile: "m", route: "project", recommendation: null }); + s.fileNeed({ bindingId: all.id, kind: "configure", url: "", what: "Enable it", how: null }); + s.ask({ projectId: "p1", bindingId: all.id, escalationId: null, question: "Which?", context: null }); + // An ended binding, and one whose escalation and need are resolved: neither counts. + const ended = add(s, 17); + s.patch(ended.id, { endedAt: 2_000 }); + const resolved = add(s, 18); + s.resolve(s.escalate({ bindingId: resolved.id, kind: "decision", question: "Q", meanwhile: "m", route: "project", recommendation: null }).id, "Done."); + s.closeNeed(s.fileNeed({ bindingId: resolved.id, kind: "configure", url: "", what: "Enable it", how: null }).id, "done"); + // A NULL status column is not the `user` actor, and another project's + // open need is not this project's. + add(s, 19); + const other = s.create({ projectId: "p2", source: "linear", entityId: "uuid-99", entityKey: "ENG-99", url: null, title: null, kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "Todo", stage: "spec" }); + s.fileNeed({ bindingId: other.id, kind: "configure", url: "", what: "Enable it", how: null }); + + const open = s.open("p1"); + const expected = open.filter((b) => needsUser(s, b)).length; + expect(expected).toBe(5); // actor user, escalation, need, question, and all four + expect(s.countNeedsUser("p1")).toBe(expected); + expect(s.countNeedsUser("p1")).not.toBe(s.countNeedsUser("p2")); + expect(s.countNeedsUser("p2")).toBe(1); + }); +}); + describe("lessons", () => { it("are recorded pending with their source, and resolved once", () => { const { s, binding, advance } = store(); diff --git a/plugins/stewards/store.ts b/plugins/stewards/store.ts index cb441aaa..e48671cb 100644 --- a/plugins/stewards/store.ts +++ b/plugins/stewards/store.ts @@ -1037,6 +1037,22 @@ export function createStore(db: Database.Database, now: () => number, options: S const allForProject = db.prepare<[string], BindingRow>("SELECT * FROM bindings WHERE project_id = ? ORDER BY created_at"); const allOpen = db.prepare<[], BindingRow>("SELECT * FROM bindings WHERE ended_at IS NULL ORDER BY created_at"); const all = db.prepare<[], BindingRow>("SELECT * FROM bindings ORDER BY created_at"); + /** + * The picker's needs count (BB-104): one statement per project, so the + * sidebar does not parse every binding and run three more queries each. It + * mirrors `needsUser`: the actor is `user`, or an open escalation, need or + * question. The open predicates are the ones `openEscalations`, `openNeeds` + * and `openQuestions` use, so the count and the yellow rows cannot disagree. + */ + const countNeedsUserStmt = db.prepare<[string], { n: number }>( + `SELECT COUNT(*) AS n FROM bindings b + WHERE b.project_id = ? AND b.ended_at IS NULL AND ( + (CASE WHEN json_valid(b.status) THEN json_extract(b.status, '$.actor') END) = 'user' + OR EXISTS (SELECT 1 FROM escalations e WHERE e.binding_id = b.id AND e.status = 'open') + OR EXISTS (SELECT 1 FROM needs n WHERE n.binding_id = b.id AND n.status = 'open') + OR EXISTS (SELECT 1 FROM escalations q WHERE q.binding_id = b.id AND q.is_question = 1 AND q.status = 'open' AND (q.resolution_kind IS NULL OR q.resolution_kind NOT IN ('secret', 'done'))) + )`, + ); const children = db.prepare<[string], BindingRow>("SELECT * FROM bindings WHERE parent_binding_id = ? AND ended_at IS NULL ORDER BY created_at"); const isPlaced = db.prepare<[string], { thread_id: string }>("SELECT thread_id FROM placed_threads WHERE thread_id = ?"); const markPlaced = db.prepare<[string, number]>("INSERT OR IGNORE INTO placed_threads (thread_id, placed_at) VALUES (?, ?)"); @@ -1618,6 +1634,10 @@ export function createStore(db: Database.Database, now: () => number, options: S all(projectId?: string): Binding[] { return (projectId === undefined ? all.all() : allForProject.all(projectId)).map(rowToBinding); }, + /** The open bindings of a project that `needsUser` would flag, counted in SQL (BB-104). */ + countNeedsUser(projectId: string): number { + return countNeedsUserStmt.get(projectId)?.n ?? 0; + }, children(bindingId: string): Binding[] { return children.all(bindingId).map(rowToBinding); }, From 7bcc29f4fde9c380ed84ede17a0b7733629b362d Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Wed, 28 Oct 2026 17:24:48 +0000 Subject: [PATCH 2/2] stewards: note the status-schema assumption in countNeedsUser --- plugins/stewards/store.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/stewards/store.ts b/plugins/stewards/store.ts index e48671cb..cfc42862 100644 --- a/plugins/stewards/store.ts +++ b/plugins/stewards/store.ts @@ -1043,6 +1043,9 @@ export function createStore(db: Database.Database, now: () => number, options: S * mirrors `needsUser`: the actor is `user`, or an open escalation, need or * question. The open predicates are the ones `openEscalations`, `openNeeds` * and `openQuestions` use, so the count and the yellow rows cannot disagree. + * It reads `$.actor` from the JSON column and so assumes a schema-valid + * `status` record or NULL; writes go through the store's typed `StatusRecord`, + * so it agrees with the zod parse behind `needsUser`. */ const countNeedsUserStmt = db.prepare<[string], { n: number }>( `SELECT COUNT(*) AS n FROM bindings b