From 719a140901f1b962f64c9cd2afad99daaebad9c3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:42:37 -0700 Subject: [PATCH] fix(miner): claimNextBatch can claim the wrong host's row across two forge hosts portfolio-queue-manager.js's caps-aware batch claiming encoded queue items into ids via bare repoFullName+identifier, with no apiBaseUrl dimension. Since #5563 scoped the underlying store by (apiBaseUrl, repoFullName, identifier), two hosts can now legitimately share a repoFullName+identifier pair -- and when they do, the engine's selection (which only ever sees the opaque id string) could select either host's item, but claimNextBatch's selectFn always defaulted target.apiBaseUrl to github.com, so batchClaim could mark a DIFFERENT row in_progress than the one actually selected. queueItemId/parseQueueItemId now encode and decode apiBaseUrl as part of the id round-trip, so claimNextBatch always claims the exact row the engine selected, never a same-name row on the wrong host. Also hardens the two already-shipped #5563 migrations (claim-ledger.js, portfolio-queue.js) with INSERT OR IGNORE for their table-rebuild copy step, matching run-state.js's and governor-state.js's later fixes for the same class of bug: a legacy row with an already-invalid status/state value (this store's own read path already fails closed on those) would violate the rebuilt table's CHECK constraint and abort the whole migration, permanently breaking that file, instead of being dropped as the corrupt garbage it already was. Advances #5563. --- packages/gittensory-miner/lib/claim-ledger.js | 6 ++- .../lib/portfolio-queue-manager.d.ts | 3 +- .../lib/portfolio-queue-manager.js | 40 ++++++++++----- .../gittensory-miner/lib/portfolio-queue.js | 6 ++- test/unit/miner-claim-ledger.test.ts | 35 +++++++++++++ .../miner-portfolio-queue-manager.test.ts | 51 ++++++++++++++++++- test/unit/miner-portfolio-queue.test.ts | 37 ++++++++++++++ 7 files changed, 160 insertions(+), 18 deletions(-) diff --git a/packages/gittensory-miner/lib/claim-ledger.js b/packages/gittensory-miner/lib/claim-ledger.js index c9e1a7267f..d6eff9a429 100644 --- a/packages/gittensory-miner/lib/claim-ledger.js +++ b/packages/gittensory-miner/lib/claim-ledger.js @@ -81,8 +81,12 @@ function addApiBaseUrlScope(db) { UNIQUE (api_base_url, repo_full_name, issue_number) ) `); + // OR IGNORE: a row this store's own read path already treats as unusable garbage (an unrecognized `status`, + // e.g. from a hand-edited or otherwise corrupted file) would violate the CHECK constraint above and abort the + // whole migration. Skipping it here is consistent with that same fail-closed posture, rather than turning one + // bad row into a permanently unmigratable file. db.prepare( - `INSERT INTO miner_claims_v2 (id, api_base_url, repo_full_name, issue_number, claimed_at, status, note) + `INSERT OR IGNORE INTO miner_claims_v2 (id, api_base_url, repo_full_name, issue_number, claimed_at, status, note) SELECT id, ?, repo_full_name, issue_number, claimed_at, status, note FROM miner_claims`, ).run(DEFAULT_FORGE_CONFIG.apiBaseUrl); db.exec("DROP TABLE miner_claims"); diff --git a/packages/gittensory-miner/lib/portfolio-queue-manager.d.ts b/packages/gittensory-miner/lib/portfolio-queue-manager.d.ts index 9dd469ff07..86ebd80cd7 100644 --- a/packages/gittensory-miner/lib/portfolio-queue-manager.d.ts +++ b/packages/gittensory-miner/lib/portfolio-queue-manager.d.ts @@ -2,11 +2,12 @@ import type { PortfolioCaps } from "@jsonbored/gittensory-engine"; import type { EnqueueItem, PortfolioQueueStore, QueueEntry } from "./portfolio-queue.js"; export type PortfolioQueueClaimTarget = { + apiBaseUrl: string; repoFullName: string; identifier: string; }; -export function queueItemId(repoFullName: string, identifier: string): string; +export function queueItemId(apiBaseUrl: string, repoFullName: string, identifier: string): string; export function parseQueueItemId(id: string): PortfolioQueueClaimTarget; diff --git a/packages/gittensory-miner/lib/portfolio-queue-manager.js b/packages/gittensory-miner/lib/portfolio-queue-manager.js index cc5d7bb25c..bd94ac2c76 100644 --- a/packages/gittensory-miner/lib/portfolio-queue-manager.js +++ b/packages/gittensory-miner/lib/portfolio-queue-manager.js @@ -3,26 +3,37 @@ // claiming respects global/per-repo WIP caps and cross-repo diversification instead of a naive priority-only // single-row dequeue. Caps are plain constructor arguments — not wired to .gittensory-miner.yml here. import { nextEligibleItems } from "@jsonbored/gittensory-engine"; +import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { initPortfolioQueueStore } from "./portfolio-queue.js"; import { DEFAULT_MAX_LEASE_MS, sweepStuckItems } from "./portfolio-queue-expiry.js"; const ITEM_ID_SEPARATOR = "::"; -/** Stable composite id for projecting SQLite rows into the engine's PortfolioQueueItem shape. */ -export function queueItemId(repoFullName, identifier) { - return `${repoFullName}${ITEM_ID_SEPARATOR}${identifier}`; +/** + * Stable composite id for projecting SQLite rows into the engine's PortfolioQueueItem shape. Encodes apiBaseUrl + * too (#5563) — the engine's own selection logic has no forge dimension, but two hosts can now enqueue an item + * under the same repoFullName+identifier (post-#5563 scoping), and the id is the ONLY thing selectEligibleBatch's + * output threads back to batchClaim; without the host baked in here, a selected item's host would be lost and + * batchClaim would default to github.com, potentially claiming a DIFFERENT row than the one the engine selected. + */ +export function queueItemId(apiBaseUrl, repoFullName, identifier) { + return `${apiBaseUrl}${ITEM_ID_SEPARATOR}${repoFullName}${ITEM_ID_SEPARATOR}${identifier}`; } /** Reverse {@link queueItemId} after engine selection so claims can target SQLite primary keys. */ export function parseQueueItemId(id) { if (typeof id !== "string") throw new Error("invalid_queue_item_id"); - const separatorIndex = id.indexOf(ITEM_ID_SEPARATOR); - if (separatorIndex <= 0 || separatorIndex === id.length - ITEM_ID_SEPARATOR.length) { + const firstSeparatorIndex = id.indexOf(ITEM_ID_SEPARATOR); + if (firstSeparatorIndex <= 0) throw new Error("invalid_queue_item_id"); + const rest = id.slice(firstSeparatorIndex + ITEM_ID_SEPARATOR.length); + const secondSeparatorIndex = rest.indexOf(ITEM_ID_SEPARATOR); + if (secondSeparatorIndex <= 0 || secondSeparatorIndex === rest.length - ITEM_ID_SEPARATOR.length) { throw new Error("invalid_queue_item_id"); } return { - repoFullName: id.slice(0, separatorIndex), - identifier: id.slice(separatorIndex + ITEM_ID_SEPARATOR.length), + apiBaseUrl: id.slice(0, firstSeparatorIndex), + repoFullName: rest.slice(0, secondSeparatorIndex), + identifier: rest.slice(secondSeparatorIndex + ITEM_ID_SEPARATOR.length), }; } @@ -42,13 +53,16 @@ export function entriesToPortfolioQueue(entries) { const repoFullName = typeof entry.repoFullName === "string" ? entry.repoFullName.trim() : ""; const identifier = typeof entry.identifier === "string" ? entry.identifier.trim() : ""; if (!repoFullName || !identifier) continue; + // 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(); if (!bucketsByRepo.has(repoKey)) { bucketsByRepo.set(repoKey, []); bucketOrder.push(repoKey); } bucketsByRepo.get(repoKey).push({ - id: queueItemId(repoFullName, identifier), + id: queueItemId(apiBaseUrl, repoFullName, identifier), repoFullName, state: entry.status === "in_progress" ? "in_progress" : "queued", }); @@ -99,12 +113,10 @@ export function initPortfolioQueueManager(options = {}) { reclaimStuckItems(maxLeaseMs = staleLeaseMs) { return sweepStuckItems(store, Date.now(), maxLeaseMs); }, - // NOTE (#5563): claimNextBatch's engine-driven selection (queueItemId/parseQueueItemId, entriesToPortfolioQueue) - // has no apiBaseUrl dimension -- @jsonbored/gittensory-engine's PortfolioQueueItem shape predates multi-forge - // support. selectFn below therefore never supplies target.apiBaseUrl, so batchClaim falls back to the - // github.com default for every claim; a non-default-host item enqueued under a different apiBaseUrl safely - // fails to match (no row, no claim, no corruption) rather than being claimed under the wrong host. Retrofitting - // the engine primitive itself with a forge dimension is out of this store-level fix's scope. + // The engine primitive itself (@jsonbored/gittensory-engine's nextEligibleItems) has no apiBaseUrl concept -- + // it only ever sees the opaque `id` string. queueItemId/parseQueueItemId (#5563) smuggle the host through + // that id round-trip, so selectFn's output below correctly carries each selected item's OWN apiBaseUrl into + // batchClaim, instead of every claim defaulting to github.com regardless of which host's row was selected. claimNextBatch() { // Reclaim orphaned leases first, so an item stranded 'in_progress' by a dead process becomes eligible again // instead of permanently consuming a WIP slot and starving the queue. diff --git a/packages/gittensory-miner/lib/portfolio-queue.js b/packages/gittensory-miner/lib/portfolio-queue.js index ff1d2bb590..d86069241e 100644 --- a/packages/gittensory-miner/lib/portfolio-queue.js +++ b/packages/gittensory-miner/lib/portfolio-queue.js @@ -132,9 +132,13 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) `); // ORDER BY rowid preserves the old table's FIFO insertion order in the new table's freshly-assigned rowids // (the composite PRIMARY KEY above is not itself the rowid), so this rebuild doesn't reshuffle queue order. + // OR IGNORE: a row this store's own read path already treats as unusable garbage (an unrecognized + // `status`, e.g. from a hand-edited or otherwise corrupted file) would violate the CHECK constraint above + // and abort the whole migration. Skipping it here is consistent with that same fail-closed posture, rather + // than turning one bad row into a permanently unmigratable file. migrationDb .prepare( - `INSERT INTO miner_portfolio_queue_v3 + `INSERT OR IGNORE INTO miner_portfolio_queue_v3 (api_base_url, repo_full_name, identifier, priority, status, enqueued_at, leased_at) SELECT ?, repo_full_name, identifier, priority, status, enqueued_at, leased_at FROM miner_portfolio_queue ORDER BY rowid`, diff --git a/test/unit/miner-claim-ledger.test.ts b/test/unit/miner-claim-ledger.test.ts index a0db7d529d..3e1192b8b2 100644 --- a/test/unit/miner-claim-ledger.test.ts +++ b/test/unit/miner-claim-ledger.test.ts @@ -316,6 +316,41 @@ describe("gittensory-miner claim ledger (#2314)", () => { expect(ledger.listClaims({ repoFullName: "acme/widgets" })).toHaveLength(2); expect(geClaim.apiBaseUrl).toBe("https://ghe.example.com/api/v3"); }); + + it("REGRESSION: a legacy row violating the rebuilt table's status CHECK constraint is dropped, not a migration-aborting crash", () => { + const root = tempRoot(); + const dbPath = join(root, "legacy-corrupt.sqlite3"); + const legacy = new DatabaseSync(dbPath); + // No CHECK on status here, simulating a hand-edited or otherwise corrupted legacy file -- the real + // baseline schema always enforces the CHECK, so this can only arise from external tampering. + legacy.exec(` + CREATE TABLE miner_claims ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_full_name TEXT NOT NULL, + issue_number INTEGER NOT NULL, + claimed_at TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'active', + note TEXT, + UNIQUE (repo_full_name, issue_number) + ) + `); + legacy.exec( + "INSERT INTO miner_claims (repo_full_name, issue_number, claimed_at, status, note) VALUES ('acme/corrupt', 1, '2026-01-01T00:00:00.000Z', 'bogus', NULL)", + ); + legacy.exec( + "INSERT INTO miner_claims (repo_full_name, issue_number, claimed_at, status, note) VALUES ('acme/widgets', 5, '2026-01-01T00:00:00.000Z', 'active', 'ok')", + ); + legacy.close(); + + let opened: ReturnType | undefined; + expect(() => { + opened = openClaimLedger(dbPath); + }).not.toThrow(); + const ledger = opened!; + ledgers.push(ledger); + // The corrupt row was dropped, not migrated -- only the valid row survived the rebuild. + expect(ledger.listClaims().map((claim) => claim.repoFullName)).toEqual(["acme/widgets"]); + }); }); describe("purgeByRepo (#5564)", () => { diff --git a/test/unit/miner-portfolio-queue-manager.test.ts b/test/unit/miner-portfolio-queue-manager.test.ts index cf4f8c50a6..9606dd60a0 100644 --- a/test/unit/miner-portfolio-queue-manager.test.ts +++ b/test/unit/miner-portfolio-queue-manager.test.ts @@ -50,12 +50,43 @@ describe("entriesToPortfolioQueue() / selectEligibleBatch() (#4285)", () => { "acme/beta", "acme/gamma", ]); - expect(parseQueueItemId(queueItemId("acme/beta", "b-queued-1"))).toEqual({ + expect(parseQueueItemId(queueItemId("https://api.github.com", "acme/beta", "b-queued-1"))).toEqual({ + apiBaseUrl: "https://api.github.com", repoFullName: "acme/beta", identifier: "b-queued-1", }); }); + it("queueItemId/parseQueueItemId round-trip a non-default apiBaseUrl (#5563)", () => { + const id = queueItemId("https://ghe.example.com/api/v3", "acme/widgets", "issue:7"); + expect(parseQueueItemId(id)).toEqual({ + apiBaseUrl: "https://ghe.example.com/api/v3", + repoFullName: "acme/widgets", + identifier: "issue:7", + }); + }); + + it("parseQueueItemId rejects a malformed id", () => { + expect(() => parseQueueItemId(42 as never)).toThrow("invalid_queue_item_id"); + expect(() => parseQueueItemId("no-separators-at-all")).toThrow("invalid_queue_item_id"); + expect(() => parseQueueItemId("https://api.github.com::acme/widgets")).toThrow("invalid_queue_item_id"); + expect(() => parseQueueItemId("::acme/widgets::issue:7")).toThrow("invalid_queue_item_id"); + expect(() => parseQueueItemId("https://api.github.com::acme/widgets::")).toThrow("invalid_queue_item_id"); + }); + + it("entriesToPortfolioQueue falls back to the github.com default when a row's apiBaseUrl is missing (#5563)", () => { + const entries = [ + { repoFullName: "acme/alpha", identifier: "x", priority: 0, status: "queued", enqueuedAt: "t1" }, + ] as QueueEntry[]; + const id = entriesToPortfolioQueue(entries).buckets[0]?.items[0]?.id; + expect(id).toBeDefined(); + expect(parseQueueItemId(id!)).toEqual({ + apiBaseUrl: "https://api.github.com", + repoFullName: "acme/alpha", + identifier: "x", + }); + }); + it("returns nothing when either cap is zero", () => { const entries: QueueEntry[] = [ { apiBaseUrl: "https://api.github.com", repoFullName: "acme/alpha", identifier: "x", priority: 0, status: "queued", enqueuedAt: "t1" }, @@ -110,6 +141,24 @@ describe("initPortfolioQueueManager().claimNextBatch() (#4285)", () => { expect(manager.listQueue().find((entry) => entry.identifier === "a-queued-2")?.status).toBe("queued"); }); + it("REGRESSION: claimNextBatch claims the correct host's row when two hosts share a repoFullName+identifier (#5563)", () => { + const manager = memoryManager({ globalWipCap: 4, perRepoWipCap: 2 }); + 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" }); + + 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", + ]); + expect(claimed.every((entry) => entry.status === "in_progress")).toBe(true); + // Every row is genuinely claimed at the store level -- not one host's row claimed twice under two ids. + const rows = manager.listQueue("acme/widgets"); + expect(rows).toHaveLength(2); + expect(rows.every((row) => row.status === "in_progress")).toBe(true); + }); + it("does not claim rows another writer already took inside the same transaction window", () => { const store = initPortfolioQueueStore(":memory:"); stores.push(store); diff --git a/test/unit/miner-portfolio-queue.test.ts b/test/unit/miner-portfolio-queue.test.ts index 9f00f8f5ff..dee0e43b03 100644 --- a/test/unit/miner-portfolio-queue.test.ts +++ b/test/unit/miner-portfolio-queue.test.ts @@ -320,5 +320,42 @@ describe("gittensory-miner portfolio/queue store (#2292)", () => { expect(store.listQueue("acme/widgets")).toHaveLength(2); expect(geEntry.apiBaseUrl).toBe("https://ghe.example.com/api/v3"); }); + + it("REGRESSION: a legacy row violating the rebuilt table's status CHECK constraint is dropped, not a migration-aborting crash", () => { + const root = mkdtempSync(join(tmpdir(), "gittensory-miner-portfolio-legacy-corrupt-")); + roots.push(root); + const dbPath = join(root, "legacy-corrupt.sqlite3"); + const legacy = new DatabaseSync(dbPath); + // No CHECK on status here, simulating a hand-edited or otherwise corrupted legacy file -- the real + // baseline schema always enforces the CHECK, so this can only arise from external tampering. + legacy.exec(` + CREATE TABLE miner_portfolio_queue ( + repo_full_name TEXT NOT NULL, + identifier TEXT NOT NULL, + priority REAL NOT NULL DEFAULT 0, + status TEXT NOT NULL DEFAULT 'queued', + enqueued_at TEXT NOT NULL, + leased_at TEXT, + PRIMARY KEY (repo_full_name, identifier) + ) + `); + legacy.exec("PRAGMA user_version = 2"); + legacy.exec( + "INSERT INTO miner_portfolio_queue (repo_full_name, identifier, priority, status, enqueued_at, leased_at) VALUES ('acme/corrupt', 'issue:1', 1, 'bogus', '2026-01-01T00:00:00.000Z', NULL)", + ); + legacy.exec( + "INSERT INTO miner_portfolio_queue (repo_full_name, identifier, priority, status, enqueued_at, leased_at) VALUES ('acme/widgets', 'issue:5', 3, 'queued', '2026-01-01T00:00:00.000Z', NULL)", + ); + legacy.close(); + + let opened: ReturnType | undefined; + expect(() => { + opened = initPortfolioQueueStore(dbPath); + }).not.toThrow(); + const store = opened!; + stores.push(store); + // The corrupt row was dropped, not migrated -- only the valid row survived the rebuild. + expect(store.listQueue().map((entry) => entry.repoFullName)).toEqual(["acme/widgets"]); + }); }); });