From 239bcfc568038653ada4bf9b7823f3726450ddfa Mon Sep 17 00:00:00 2001 From: Nikan Wystaf Date: Sat, 26 Sep 2026 16:33:25 +0700 Subject: [PATCH] perf(providers): make POST /api/providers O(1) and refuse silent key overwrite (#4350) Fixes #4311 - Drop full-pool renumber on insert: new row gets MAX(priority)+1 directly, turning an O(pool) rewrite into O(1) per insert. - For apikey connections, query by (provider, authType, name) and count via SQL aggregate instead of reading the entire pool into memory. - Refuse silent apikey overwrite on name collision with 409 PROVIDER_NAME_CONFLICT, unless caller explicitly sets allowOverwrite: true. - Add 8 unit tests covering priority ordering and name collisions. --- src/app/api/providers/route.js | 9 ++ src/lib/db/repos/connectionsRepo.js | 60 +++++++- .../provider-priority-insert-cost.test.js | 138 ++++++++++++++++++ 3 files changed, 201 insertions(+), 6 deletions(-) create mode 100644 tests/unit/provider-priority-insert-cost.test.js diff --git a/src/app/api/providers/route.js b/src/app/api/providers/route.js index 5885472b..49f611aa 100644 --- a/src/app/api/providers/route.js +++ b/src/app/api/providers/route.js @@ -183,6 +183,9 @@ export async function POST(request) { providerSpecificData: mergedProviderSpecificData, isActive: true, testStatus: testStatus || "unknown", + // POST with an id is an explicit edit of that connection; without one, a + // name collision is refused rather than silently overwriting a key. #4311 + allowOverwrite: body.id ? true : (body.allowOverwrite === true || body.overwrite === true), }); // Hide sensitive fields @@ -191,6 +194,12 @@ export async function POST(request) { return NextResponse.json({ connection: result }, { status: 201 }); } catch (error) { + if (error?.code === "PROVIDER_NAME_CONFLICT") { + return NextResponse.json( + { error: error.message, code: error.code, existingId: error.existingId, existingName: error.existingName }, + { status: 409 } + ); + } console.log("Error creating provider:", error); return NextResponse.json({ error: "Failed to create provider" }, { status: 500 }); } diff --git a/src/lib/db/repos/connectionsRepo.js b/src/lib/db/repos/connectionsRepo.js index 78abfc90..5c51aeaa 100644 --- a/src/lib/db/repos/connectionsRepo.js +++ b/src/lib/db/repos/connectionsRepo.js @@ -108,7 +108,15 @@ export async function getProviderConnectionById(id) { return rowToConn(row); } -// Internal sync reorder — must be called INSIDE a transaction +// Internal sync reorder — must be called INSIDE a transaction. +// +// Normalizes priorities to a contiguous 1..N after a DELETE or an explicit +// reorder, so gaps don't accumulate over time. +// +// Deliberately NOT called on insert: a new connection already gets +// MAX(priority)+1, which sorts after every existing row, so the order is +// identical with or without the rewrite. Skipping it there is what makes +// import O(1) per key instead of O(pool) — see createProviderConnection. function reorderInTx(db, providerId) { const list = db.all(`SELECT * FROM providerConnections WHERE provider = ?`, [providerId]).map(rowToConn); list.sort((a, b) => { @@ -117,7 +125,10 @@ function reorderInTx(db, providerId) { return new Date(b.updatedAt || 0) - new Date(a.updatedAt || 0); }); list.forEach((c, i) => { - db.run(`UPDATE providerConnections SET priority = ? WHERE id = ?`, [i + 1, c.id]); + const want = i + 1; + if ((c.priority || 0) !== want) { + db.run(`UPDATE providerConnections SET priority = ? WHERE id = ?`, [want, c.id]); + } }); } @@ -127,7 +138,21 @@ export async function createProviderConnection(data) { let result; db.transaction(() => { - const all = db.all(`SELECT * FROM providerConnections WHERE provider = ?`, [data.provider]).map(rowToConn); + // apikey connections are deduped by name and need only the current max + // priority, so query for those directly instead of loading the whole pool + // (O(pool) per key — the other half of the import cost in #4311). The oauth + // branch below still scans, because its identity rules compare fields + // inside providerSpecificData and have no single-column equivalent. + const isApikey = data.authType === "apikey" && !!data.name; + const all = isApikey + ? db.all( + `SELECT * FROM providerConnections WHERE provider = ? AND authType = ? AND name = ?`, + [data.provider, "apikey", data.name] + ).map(rowToConn) + : db.all(`SELECT * FROM providerConnections WHERE provider = ?`, [data.provider]).map(rowToConn); + const poolSize = isApikey + ? db.get(`SELECT COUNT(*) AS n FROM providerConnections WHERE provider = ?`, [data.provider])?.n ?? all.length + : all.length; let existing = null; if (data.authType === "oauth" && data.email) { @@ -169,6 +194,21 @@ export async function createProviderConnection(data) { // access_token: never dedup — user manages duplicates manually if (existing) { + // Name collision on an apikey connection used to silently replace the + // stored apiKey, so a script that reused names ("Key 1", "Key 2", …) + // destroyed existing pool entries with no 409 and no warning. Callers that + // genuinely mean "update this one" pass allowOverwrite; everyone else gets + // a typed error naming the row that would have been replaced. #4311 + if (data.allowOverwrite === false) { + const err = new Error( + `A connection named "${existing.name}" already exists for provider "${data.provider}". ` + + `Pass allowOverwrite: true to replace it.` + ); + err.code = "PROVIDER_NAME_CONFLICT"; + err.existingId = existing.id; + err.existingName = existing.name; + throw err; + } const normalized = resetHealthStateOnActivation(existing, data); const merged = { ...existing, ...normalized, updatedAt: now }; upsert(db, merged); @@ -178,11 +218,15 @@ export async function createProviderConnection(data) { let connectionName = data.name || null; if (!connectionName && (data.authType === "oauth" || data.authType === "access_token")) { - connectionName = deriveConnectionName(data, data.email || `Account ${all.length + 1}`); + connectionName = deriveConnectionName(data, data.email || `Account ${poolSize + 1}`); } let connectionPriority = data.priority; if (!connectionPriority) { - connectionPriority = all.reduce((m, c) => Math.max(m, c.priority || 0), 0) + 1; + // MAX(priority)+1 in SQL rather than a reduce over the loaded pool: the + // apikey path no longer has the whole pool in memory, and the aggregate + // is served by the index instead of a row scan. #4311 + const maxRow = db.get(`SELECT MAX(priority) AS m FROM providerConnections WHERE provider = ?`, [data.provider]); + connectionPriority = (maxRow?.m || 0) + 1; } const conn = { @@ -204,7 +248,11 @@ export async function createProviderConnection(data) { if (data.email !== undefined) conn.email = data.email; upsert(db, conn); - reorderInTx(db, data.provider); + // No reorderInTx here. `conn.priority` is already MAX(priority)+1, so the + // row sorts last and the resulting order is what reorderInTx would have + // produced anyway. The rewrite cost ~2N statements per insert — O(pool) — + // which made a 5k-key import O(n*m): ~25M statements at a 5k pool, and it + // serialized every parallel writer on the same transaction. #4311 result = conn; }); diff --git a/tests/unit/provider-priority-insert-cost.test.js b/tests/unit/provider-priority-insert-cost.test.js new file mode 100644 index 00000000..bd2f2ef6 --- /dev/null +++ b/tests/unit/provider-priority-insert-cost.test.js @@ -0,0 +1,138 @@ +import { describe, expect, it } from "vitest"; + +import { + createProviderConnection, + getProviderConnections, + deleteProviderConnection, + updateProviderConnection, +} from "../../src/lib/db/index.js"; + +// #4311: POST /api/providers was O(pool) per insert. Inside one transaction it +// read the whole pool AND renumbered every row's priority, so a 5k-key import +// was O(n*m) — ~25M statements at a 5k pool — and every parallel writer +// serialized on the same transaction. On top of that, an apikey name collision +// silently overwrote the stored key with no 409. +// +// The test DB persists across tests in a file, so each case uses its own +// provider alias; priorities are per-provider. + +async function seed(provider, n) { + for (let i = 0; i < n; i++) { + await createProviderConnection({ + provider, + authType: "apikey", + name: `seed-${i}`, + apiKey: `k${i}`, + }); + } +} + +describe("provider insert is O(1) in pool size (#4311)", () => { + it("assigns sequential priorities without a renumber pass", async () => { + const P = `openai-compatible-seq-${Date.now()}`; + await seed(P, 3); + const list = await getProviderConnections({ provider: P }); + expect(list.map((c) => c.name)).toEqual(["seed-0", "seed-1", "seed-2"]); + expect(list.map((c) => c.priority)).toEqual([1, 2, 3]); + }); + + it("keeps a large pool in insertion order", async () => { + const P = `openai-compatible-ord-${Date.now()}`; + await seed(P, 60); + const list = await getProviderConnections({ provider: P }); + expect(list).toHaveLength(60); + // The bug showed up as reordering once the pool grew past a few rows. + expect(list[0].name).toBe("seed-0"); + expect(list[59].name).toBe("seed-59"); + for (let i = 1; i < list.length; i++) { + expect(list[i].priority).toBeGreaterThan(list[i - 1].priority); + } + }); + + it("still renumbers on delete, so gaps do not accumulate", async () => { + const P = `openai-compatible-del-${Date.now()}`; + await seed(P, 4); + const before = await getProviderConnections({ provider: P }); + await deleteProviderConnection(before[0].id); + const after = await getProviderConnections({ provider: P }); + expect(after.map((c) => c.priority)).toEqual([1, 2, 3]); + }); + + it("still renumbers on an explicit priority update", async () => { + // Unique alias per run: the DB persists across runs, so a fixed alias + // would accumulate rows and make this assertion depend on test order. + const P = `openai-compatible-upd-${Date.now()}`; + await seed(P, 4); + await new Promise((r) => setTimeout(r, 10)); + const list = await getProviderConnections({ provider: P }); + // Move the last one to the front. + await updateProviderConnection(list[3].id, { priority: 1 }); + const after = await getProviderConnections({ provider: P }); + expect(after[0].name).toBe("seed-3"); + }); +}); + +describe("name collision no longer destroys a key silently (#4311)", () => { + // Seeded once: these cases each mutate the SAME row, so a per-test seed + // would make the later assertions depend on earlier ones. + const P = `openai-compatible-clash-${Date.now()}`; + const original = (async () => { + await seed(P, 1); + return (await getProviderConnections({ provider: P }))[0]; + })(); + + it("throws a typed conflict instead of overwriting, when overwrite is refused", async () => { + const orig = await original; + await expect( + createProviderConnection({ + provider: P, + authType: "apikey", + name: orig.name, + apiKey: "REPLACEMENT-KEY", + allowOverwrite: false, + }) + ).rejects.toMatchObject({ code: "PROVIDER_NAME_CONFLICT", existingId: orig.id }); + + // The stored key must be untouched. + const after = (await getProviderConnections({ provider: P }))[0]; + expect(after.apiKey).toBe(orig.apiKey); + }); + + it("still overwrites when the caller opts in", async () => { + const orig = await original; + const updated = await createProviderConnection({ + provider: P, + authType: "apikey", + name: orig.name, + apiKey: "REPLACEMENT-KEY", + allowOverwrite: true, + }); + expect(updated.id).toBe(orig.id); + const after = (await getProviderConnections({ provider: P }))[0]; + expect(after.apiKey).toBe("REPLACEMENT-KEY"); + }); + + it("defaults to the previous overwrite behaviour for existing callers", async () => { + // Every other call site in the repo (oauth routes, bulk import) omits the + // flag, so they must keep working exactly as before. + const orig = await original; + const updated = await createProviderConnection({ + provider: P, + authType: "apikey", + name: orig.name, + apiKey: "LEGACY-PATH-KEY", + }); + expect(updated.id).toBe(orig.id); + }); + + it("does not collide across different providers", async () => { + const orig = await original; + const other = await createProviderConnection({ + provider: "openai-compatible-other", + authType: "apikey", + name: orig.name, + apiKey: "other-key", + }); + expect(other.id).not.toBe(orig.id); + }); +});