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"]); + }); }); });