stewards: refresh project configs off the read path and count picker needs in SQL (BB-104)

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<ReturnType<typeof setup>>) => {
+    const original = t.sdk.plugins.callRpc;
+    const reads: string[][] = [];
+    let gate: Promise<void> | 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<void>((resolve) => {
+          release = resolve;
+        });
+      },
+      release() {
+        release();
+        gate = null;
+      },
+    };
+  };
+
+  const load = (t: Awaited<ReturnType<typeof setup>>, 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;

plugins/stewards/server.ts

@@ -890,32 +890,61 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P
     // ---- configuration ----------------------------------------------------------------
 
     const configs = new Map<string, { at: number; value: ProjectConfig | null }>();
+    /**
+     * 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<string, Promise<ProjectConfig | null>>();
 
     async function readFiles(projectId: string, paths: string[]): Promise<Map<string, string | null>> {
       const out = await rpc(MULTI_REPO_PLUGIN_ID, "readProjectFiles", { projectId, paths }, readFilesOutput);
       if (out.error !== null && out.files.length === 0) throw new Error(out.error);
       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<ProjectConfig | null> {
+      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<ProjectConfig | null> {
       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<ProjectConfig | null> {
@@ -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<string, { sweep(): Promise<void>; requeueTick(): Promise<void>; unhideOnce(): Promise<void>; store: Store; db: { prepare(sql: string): { run(...args: unknown[]): unknown } }; settings: { get(): Promise<Record<string, unknown>> }; forgetTags(): void; nightlyPass(projectId: string, night: string): Promise<void>; settle(): Promise<void>; sentryDoorbell(id: string, unit: string | null): void; sentryLeases(): Lease[] }>();
+export const testHooks = new Map<string, { sweep(): Promise<void>; requeueTick(): Promise<void>; unhideOnce(): Promise<void>; store: Store; db: { prepare(sql: string): { run(...args: unknown[]): unknown } }; settings: { get(): Promise<Record<string, unknown>> }; forgetTags(): void; nightlyPass(projectId: string, night: string): Promise<void>; loadConfig(projectId: string, force?: boolean): Promise<ProjectConfig | null>; forgetConfigs(): void; settle(): Promise<void>; sentryDoorbell(id: string, unit: string | null): void; sentryLeases(): Lease[] }>();
 
 export { DEFAULT_PLAYBOOKS, STEWARDS_PLUGIN_ID };