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

@@ -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 });
   }

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;
   });
 

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