From e445bc8087c272f1222866a2e034ab25eb9acd9b Mon Sep 17 00:00:00 2001 From: xfodev Date: Sun, 19 Jul 2026 03:58:54 -0700 Subject: [PATCH] fix(portfolio): scope per-repo WIP caps by forge host, not repo name alone (#7224) --- .../loopover-miner/lib/portfolio-queue-cli.js | 8 +++++- .../lib/portfolio-queue-manager.js | 26 +++++++++++++------ test/unit/miner-portfolio-queue-cli.test.ts | 12 +++++++++ .../miner-portfolio-queue-manager.test.ts | 15 +++++++++++ 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/packages/loopover-miner/lib/portfolio-queue-cli.js b/packages/loopover-miner/lib/portfolio-queue-cli.js index 7fb52b2956..96a6e8e6dc 100644 --- a/packages/loopover-miner/lib/portfolio-queue-cli.js +++ b/packages/loopover-miner/lib/portfolio-queue-cli.js @@ -118,8 +118,14 @@ export function selectNextEligibleTarget(entries, caps) { } const globalActiveCount = entries.filter((entry) => entry.status === "in_progress").length; if (globalActiveCount >= caps.globalWipCap) return []; + // Host-scope the per-repo active count (#7224): a same-named repo on a DIFFERENT forge host is a distinct backlog + // (the store keys rows by apiBaseUrl too, #5563), so an in-progress item on host A must not consume host B's + // per-repo WIP budget. Single-host is unchanged: every entry shares one apiBaseUrl, so the added match is always true. const repoActiveCount = entries.filter( - (entry) => entry.status === "in_progress" && entry.repoFullName === topQueued.repoFullName, + (entry) => + entry.status === "in_progress" && + entry.repoFullName === topQueued.repoFullName && + entry.apiBaseUrl === topQueued.apiBaseUrl, ).length; if (repoActiveCount >= caps.perRepoWipCap) return []; return [{ repoFullName: topQueued.repoFullName, identifier: topQueued.identifier, apiBaseUrl: topQueued.apiBaseUrl }]; diff --git a/packages/loopover-miner/lib/portfolio-queue-manager.js b/packages/loopover-miner/lib/portfolio-queue-manager.js index b9ab3e8887..c82617c54a 100644 --- a/packages/loopover-miner/lib/portfolio-queue-manager.js +++ b/packages/loopover-miner/lib/portfolio-queue-manager.js @@ -56,22 +56,32 @@ export function entriesToPortfolioQueue(entries) { // Falls back to the github.com default (matching every store's own normalizeApiBaseUrl) so a row from // before #5563 threaded apiBaseUrl through this fold still gets a valid, host-scoped id. const apiBaseUrl = typeof entry.apiBaseUrl === "string" && entry.apiBaseUrl.trim() ? entry.apiBaseUrl.trim() : DEFAULT_FORGE_CONFIG.apiBaseUrl; - const repoKey = repoFullName.toLowerCase(); + // Host-qualify the engine's per-repo WIP grouping key (#7224). nextEligibleItems groups its per-repo cap by each + // item's `repoFullName`, which it treats as an OPAQUE string -- the engine has no apiBaseUrl concept, the host is + // smuggled through the opaque `id` (queueItemId, #5563). The store keys rows by apiBaseUrl too, so two forge + // hosts' same-named repos are distinct backlogs; without qualifying the grouping key by host here, a per-repo cap + // was shared across them (e.g. perRepoWipCap: 1 let only ONE host's backlog advance). The `id` still carries the + // TRUE repoFullName and selectEligibleBatch maps results back via parseQueueItemId(id), so the real repo/host + // survive to the caller. Single-host behavior is unchanged: one apiBaseUrl means one grouping key per repo. + const repoLower = repoFullName.toLowerCase(); + const repoKey = `${apiBaseUrl}\n${repoLower}`; if (!bucketsByRepo.has(repoKey)) { - bucketsByRepo.set(repoKey, []); + // The bucket's own repoFullName stays the plain repo (display/diversification), while each ITEM carries the + // host-qualified key the engine groups on -- so the returned bucket shape is unchanged for single-host. + bucketsByRepo.set(repoKey, { repoFullName: repoLower, items: [] }); bucketOrder.push(repoKey); } - bucketsByRepo.get(repoKey).push({ + bucketsByRepo.get(repoKey).items.push({ id: queueItemId(apiBaseUrl, repoFullName, identifier), - repoFullName, + repoFullName: repoKey, state: entry.status === "in_progress" ? "in_progress" : "queued", }); } return { - buckets: bucketOrder.map((repoFullName) => ({ - repoFullName, - items: bucketsByRepo.get(repoFullName), - })), + buckets: bucketOrder.map((repoKey) => { + const bucket = bucketsByRepo.get(repoKey); + return { repoFullName: bucket.repoFullName, items: bucket.items }; + }), }; } diff --git a/test/unit/miner-portfolio-queue-cli.test.ts b/test/unit/miner-portfolio-queue-cli.test.ts index a0740cb52e..e694f34846 100644 --- a/test/unit/miner-portfolio-queue-cli.test.ts +++ b/test/unit/miner-portfolio-queue-cli.test.ts @@ -210,6 +210,18 @@ describe("loopover-miner portfolio queue CLI (#2292)", () => { { apiBaseUrl: "https://api.github.com", repoFullName: "acme/other", identifier: "b1" }, ]); }); + + it("REGRESSION: an in-progress item on one host does not consume another host's per-repo WIP budget (#7224)", () => { + const entries = [ + entry({ status: "in_progress", identifier: "host-a", apiBaseUrl: "https://api.github.com" }), + entry({ status: "queued", identifier: "host-b", apiBaseUrl: "https://ghe.example.com/api/v3" }), + ]; + // Same repo name, cap 1 — but the in-progress item is on a DIFFERENT forge host, so host B's queued row is + // eligible. Before #7224 the host-A item was mis-counted against host B's cap and this returned []. + expect(selectNextEligibleTarget(entries, { globalWipCap: 5, perRepoWipCap: 1 })).toEqual([ + { apiBaseUrl: "https://ghe.example.com/api/v3", repoFullName: "acme/widgets", identifier: "host-b" }, + ]); + }); }); it("runQueueNext with --global-wip/--per-repo-wip claims only within the configured caps (#4850)", () => { diff --git a/test/unit/miner-portfolio-queue-manager.test.ts b/test/unit/miner-portfolio-queue-manager.test.ts index 3d45f89a92..332c953db4 100644 --- a/test/unit/miner-portfolio-queue-manager.test.ts +++ b/test/unit/miner-portfolio-queue-manager.test.ts @@ -170,6 +170,21 @@ describe("initPortfolioQueueManager().claimNextBatch() (#4285)", () => { expect(rows.every((row) => row.status === "in_progress")).toBe(true); }); + it("REGRESSION: a binding per-repo WIP cap of 1 still lets two hosts' same-named repos each advance (#7224)", () => { + const manager = memoryManager({ globalWipCap: 4, perRepoWipCap: 1 }); + manager.enqueue({ repoFullName: "acme/widgets", identifier: "issue:1", priority: 1, apiBaseUrl: "https://api.github.com" }); + manager.enqueue({ repoFullName: "acme/widgets", identifier: "issue:1", priority: 1, apiBaseUrl: "https://ghe.example.com/api/v3" }); + + // The cap binds PER HOST: each host's independent backlog gets its one claim. Before #7224, the two hosts were + // bucketed together under the bare repoFullName, so a cap of 1 let only ONE of them advance. + const claimed = manager.claimNextBatch(); + expect(claimed).toHaveLength(2); + expect(claimed.map((entry) => entry.apiBaseUrl).sort()).toEqual([ + "https://api.github.com", + "https://ghe.example.com/api/v3", + ]); + }); + it("does not claim rows another writer already took inside the same transaction window", () => { const store = initPortfolioQueueStore(":memory:"); stores.push(store);