fix(api): bulk-add API keys no longer overwrite existing connections (#7234)

* fix(api): bulk-add API keys no longer overwrite existing connections

createProviderConnection upserts apikey connections BY NAME (same provider +
auth_type "apikey" + same name updates the row in place, replacing its
apiKey/priority/testStatus instead of inserting). The bulk-add route
auto-names unnamed lines "Key 1", "Key 2", ... restarting from 1 on every
request, blind to names already saved for the provider — so re-running a
bulk paste against a provider that already had "Key 1" silently replaced it
instead of adding a new connection alongside it. The same collision could
also happen within one batch for two identical custom name|apiKey lines.

Add resolveBulkNameCollisions (src/shared/utils/bulkApiKeyParser.ts): gap-fills
the smallest free "<name> <n>" suffix against both existing connection names
and names already assigned earlier in the same batch, so a name is never
reused. Wire it into POST /api/providers/bulk before the create loop, fetching
existing apikey connection names via the existing getProviderConnections db
module (no raw SQL added to the route).

Co-authored-by: asynx6 <sahrulbeni656@gmail.com>
Inspired-by: https://github.com/decolua/9router/pull/2587

* chore(changelog): fragment for #7234

---------

Co-authored-by: asynx6 <sahrulbeni656@gmail.com>

changelog.d/fixes/7234-bulk-add-keys-no-overwrite.md

@@ -0,0 +1 @@
+- **api:** bulk-add API keys no longer overwrite existing provider connections — a colliding auto- or custom-generated name now gap-fills a free suffix instead of silently replacing a saved connection's key/state. (thanks @asynx6)

src/app/api/providers/bulk/route.ts

@@ -4,13 +4,19 @@ import {
   getProviderAuditTarget,
   summarizeProviderConnectionForAudit,
 } from "@/lib/compliance/providerAudit";
-import { createProviderConnection, getProviderNodeById, isCloudEnabled } from "@/models";
+import {
+  createProviderConnection,
+  getProviderConnections,
+  getProviderNodeById,
+  isCloudEnabled,
+} from "@/models";
 import {
   isAnthropicCompatibleProvider,
   isOpenAICompatibleProvider,
   supportsBulkApiKey,
 } from "@/shared/constants/providers";
 import { getConsistentMachineId } from "@/shared/utils/machineId";
+import { resolveBulkNameCollisions } from "@/shared/utils/bulkApiKeyParser";
 import { syncToCloud } from "@/lib/cloudSync";
 import { bulkCreateProviderSchema } from "@/shared/validation/schemas";
 import { isValidationFailure, validateBody } from "@/shared/validation/helpers";
@@ -103,11 +109,23 @@ export async function POST(request: Request) {
       null
     : null;
 
+  // #2587 — createProviderConnection upserts apikey connections BY NAME, so a
+  // bulk-add name that collides with an already-saved connection (or with
+  // another entry in the same batch) would silently REPLACE that connection's
+  // apiKey/priority/testStatus instead of inserting a new one. Resolve every
+  // collision up front by gap-filling a free "<name> <n>" suffix so each entry
+  // reaches createProviderConnection as a genuine insert.
+  const existingConnections = await getProviderConnections({ provider, authType: "apikey" });
+  const existingNames = existingConnections
+    .map((c) => (typeof c.name === "string" ? c.name : null))
+    .filter((n): n is string => !!n);
+  const resolvedEntries = resolveBulkNameCollisions(entries, existingNames);
+
   const created: Array<Record<string, unknown>> = [];
   const errors: Array<{ index: number; name: string; message: string }> = [];
 
-  for (let i = 0; i < entries.length; i++) {
-    const entry = entries[i];
+  for (let i = 0; i < resolvedEntries.length; i++) {
+    const entry = resolvedEntries[i];
     try {
       // Per-entry copy so each connection gets its own providerSpecificData. Cloudflare
       // Workers AI carries a per-key accountId (name|accountId|apiKey) that must NOT bleed

src/shared/utils/bulkApiKeyParser.ts

@@ -111,3 +111,57 @@ export function parseBulkApiKeys(
 }
 
 export const BULK_API_KEY_MAX_LINES = MAX_BULK_LINES;
+
+// Strips a trailing " <digits>" suffix so a colliding name's numeric index can be
+// regenerated instead of stacking (e.g. "Key 1" -> "Key", not "Key 1 2").
+function stripTrailingIndex(name: string): string {
+  const stripped = name.replace(/\s+\d+$/, "");
+  return stripped.length > 0 ? stripped : name;
+}
+
+/**
+ * Resolves name collisions across a batch of bulk-add entries.
+ *
+ * Background: `createProviderConnection` upserts apikey connections BY NAME
+ * (see `src/lib/db/providers.ts` — same provider + auth_type "apikey" + same
+ * `name` updates the existing row instead of inserting a new one, replacing
+ * its `apiKey`/`priority`/`testStatus`). `parseBulkApiKeys` auto-names
+ * unnamed lines "Key 1", "Key 2", ... per request, blind to names already
+ * saved for the provider — and a batch can also contain the same custom
+ * `name|apiKey` name twice. Either case previously reached the backend upsert
+ * path and silently overwrote (or self-collapsed) an existing connection
+ * instead of inserting a new one.
+ *
+ * This resolves every collision — against `existingNames` AND against names
+ * already assigned earlier in the same batch — by gap-filling the smallest
+ * free "<base> <n>" suffix, so a name is never reused and every entry reaches
+ * the backend as a genuine insert.
+ */
+export function resolveBulkNameCollisions<T extends { name: string }>(
+  entries: T[],
+  existingNames: readonly string[] | null | undefined
+): T[] {
+  const used = new Set(
+    (Array.isArray(existingNames) ? existingNames : [])
+      .filter((n): n is string => typeof n === "string" && n.length > 0)
+      .map((n) => n.toLowerCase())
+  );
+
+  return entries.map((entry) => {
+    const lowerName = entry.name.toLowerCase();
+    if (!used.has(lowerName)) {
+      used.add(lowerName);
+      return entry;
+    }
+
+    const base = stripTrailingIndex(entry.name);
+    let idx = 1;
+    let candidate = `${base} ${idx}`;
+    while (used.has(candidate.toLowerCase())) {
+      idx += 1;
+      candidate = `${base} ${idx}`;
+    }
+    used.add(candidate.toLowerCase());
+    return { ...entry, name: candidate };
+  });
+}

tests/unit/bulk-add-keys-no-overwrite-2587.test.ts

@@ -0,0 +1,174 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+
+type ConnectionRow = Record<string, unknown> & { id: string; name?: string | null };
+
+// #2587 — bulk-add API keys must APPEND connections, never silently overwrite
+// an existing one. `createProviderConnection` upserts apikey connections BY
+// NAME (src/lib/db/providers.ts): same provider + auth_type "apikey" + same
+// `name` updates the existing row (replacing its apiKey/priority/testStatus)
+// instead of inserting a new one. Bulk-add auto-names unnamed lines
+// "Key 1", "Key 2", ... starting fresh on every request, blind to names
+// already saved for the provider — so re-running a bulk paste against a
+// provider that already has "Key 1" silently replaced it instead of adding a
+// new connection alongside it.
+//
+// The fix (POST /api/providers/bulk, src/app/api/providers/bulk/route.ts)
+// fetches existing connection names for the provider and runs
+// `resolveBulkNameCollisions` (src/shared/utils/bulkApiKeyParser.ts) before
+// calling createProviderConnection, gap-filling a free "<name> <n>" suffix so
+// every entry reaches createProviderConnection as a genuine insert. This test
+// exercises that exact production sequence — getProviderConnections ->
+// resolveBulkNameCollisions -> createProviderConnection — against a real
+// SQLite-backed db module.
+
+const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-bulk-add-2587-"));
+process.env.DATA_DIR = TEST_DATA_DIR;
+
+const core = await import("../../src/lib/db/core.ts");
+const providersDb = await import("../../src/lib/db/providers.ts");
+const { resolveBulkNameCollisions } = await import("../../src/shared/utils/bulkApiKeyParser.ts");
+
+async function resetStorage() {
+  core.resetDbInstance();
+  for (let attempt = 0; attempt < 10; attempt++) {
+    try {
+      if (fs.existsSync(TEST_DATA_DIR)) {
+        fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
+      }
+      break;
+    } catch (error) {
+      const code = (error as { code?: string } | undefined)?.code;
+      if ((code === "EBUSY" || code === "EPERM") && attempt < 9) {
+        await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1)));
+      } else {
+        throw error;
+      }
+    }
+  }
+  fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
+}
+
+test.beforeEach(async () => {
+  await resetStorage();
+});
+
+test.after(async () => {
+  core.resetDbInstance();
+  fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
+});
+
+test("bulk-add appends N+M connections and preserves the existing connection's state (the #2587 fix)", async () => {
+  // Existing connection carries live resilience state: an active rate-limit
+  // cooldown, a recorded error, and a non-default backoff level/priority.
+  const future = new Date(Date.now() + 60_000).toISOString();
+  const created = await providersDb.createProviderConnection({
+    provider: "openai",
+    authType: "apikey",
+    name: "Key 1",
+    apiKey: "sk-existing",
+    priority: 3,
+    testStatus: "unavailable",
+    lastError: "429 rate limited",
+    lastErrorType: "rate_limit",
+    rateLimitedUntil: future,
+  });
+  assert.ok(created, "existing connection must be created");
+  // `backoffLevel` is set by the resilience/cooldown path (not at creation
+  // time) — apply it the same way a real cooldown escalation would.
+  const existing = await providersDb.updateProviderConnection(created!.id as string, {
+    backoffLevel: 2,
+  });
+  assert.ok(existing, "existing connection must be updatable");
+
+  const before = await providersDb.getProviderConnections({ provider: "openai" });
+  assert.equal(before.length, 1);
+
+  // Simulates a fresh bulk-add paste (parseBulkApiKeys restarts auto-naming at
+  // "Key 1" every request) — this is exactly what collides with the existing
+  // connection above.
+  const rawEntries = [
+    { name: "Key 1", apiKey: "sk-new-1" },
+    { name: "Key 2", apiKey: "sk-new-2" },
+  ];
+
+  // Reproduces the route's fix: fetch existing apikey names for the provider,
+  // then resolve collisions before ever calling createProviderConnection.
+  const existingApikeyConnections = (await providersDb.getProviderConnections({
+    provider: "openai",
+    authType: "apikey",
+  })) as ConnectionRow[];
+  const existingNames = existingApikeyConnections
+    .map((c) => (typeof c.name === "string" ? c.name : null))
+    .filter((n): n is string => !!n);
+  const resolvedEntries = resolveBulkNameCollisions(rawEntries, existingNames);
+
+  // Neither new entry may reuse the existing connection's name.
+  assert.ok(!resolvedEntries.some((e) => e.name === "Key 1"));
+
+  for (const entry of resolvedEntries) {
+    const created = await providersDb.createProviderConnection({
+      provider: "openai",
+      authType: "apikey",
+      name: entry.name,
+      apiKey: entry.apiKey,
+      priority: 1,
+      testStatus: "unknown",
+    });
+    assert.ok(created);
+  }
+
+  const after = (await providersDb.getProviderConnections({
+    provider: "openai",
+  })) as ConnectionRow[];
+  // N (1 existing) + M (2 new) = 3 connections — never 2 (an insert masquerading
+  // as an overwrite) or 1 (both new entries collapsing into the existing row).
+  assert.equal(after.length, before.length + rawEntries.length);
+
+  const survivor = after.find((c) => c.id === (existing as ConnectionRow).id);
+  assert.ok(survivor, "the pre-existing connection must still exist, unreplaced");
+  assert.equal(survivor!.name, "Key 1");
+  assert.equal(survivor!.apiKey, "sk-existing", "existing apiKey must not be overwritten");
+  assert.equal(survivor!.priority, 3, "existing priority must survive the bulk-add");
+  assert.equal(survivor!.testStatus, "unavailable", "existing testStatus must survive");
+  assert.equal(survivor!.lastError, "429 rate limited", "existing lastError must survive");
+  assert.equal(survivor!.rateLimitedUntil, future, "existing cooldown must survive");
+  assert.equal(survivor!.backoffLevel, 2, "existing backoffLevel must survive");
+
+  const newNames = after
+    .filter((c) => c.id !== (existing as ConnectionRow).id)
+    .map((c) => c.name);
+  assert.equal(new Set(newNames).size, newNames.length, "no duplicate names among new entries");
+  assert.ok(!newNames.includes("Key 1"));
+});
+
+test("without collision resolution, a colliding bulk entry silently replaces the existing connection (documents the pre-fix bug)", async () => {
+  // This is the exact bug reported upstream: createProviderConnection's
+  // name-based upsert is intentionally unchanged (other single-add/import
+  // flows depend on it) — the guard lives one layer up, in the bulk route.
+  // Skipping that guard reproduces the original data-loss behavior.
+  const existing = await providersDb.createProviderConnection({
+    provider: "anthropic",
+    authType: "apikey",
+    name: "Key 1",
+    apiKey: "sk-existing",
+  });
+
+  const collided = await providersDb.createProviderConnection({
+    provider: "anthropic",
+    authType: "apikey",
+    name: "Key 1", // same name, no collision resolution applied
+    apiKey: "sk-overwritten",
+  });
+
+  assert.equal(collided!.id, existing!.id, "same name upserts into the same row");
+
+  const rows = (await providersDb.getProviderConnections({
+    provider: "anthropic",
+  })) as ConnectionRow[];
+  assert.equal(rows.length, 1, "no new connection was inserted — this is the bug");
+  assert.equal(rows[0].apiKey, "sk-overwritten", "the original key was overwritten");
+});

tests/unit/bulkApiKeyParser.test.ts

@@ -2,6 +2,7 @@ import test from "node:test";
 import assert from "node:assert/strict";
 import {
   parseBulkApiKeys,
+  resolveBulkNameCollisions,
   BULK_API_KEY_MAX_LINES,
 } from "../../src/shared/utils/bulkApiKeyParser.ts";
 
@@ -87,3 +88,66 @@ test("input exceeding cap is truncated with warning", () => {
   assert.equal(warnings.length, 1);
   assert.match(warnings[0], /only the first/);
 });
+
+// #2587 — resolveBulkNameCollisions: guards against createProviderConnection's
+// name-based upsert silently replacing an existing connection when bulk-add
+// auto-naming ("Key 1", "Key 2", ...) restarts from 1 and collides with a
+// name already saved for the provider.
+test("resolveBulkNameCollisions: no-op when nothing collides", () => {
+  const entries = [{ name: "Key 1" }, { name: "Key 2" }];
+  const out = resolveBulkNameCollisions(entries, ["Prod"]);
+  assert.deepEqual(
+    out.map((e) => e.name),
+    ["Key 1", "Key 2"]
+  );
+});
+
+test("resolveBulkNameCollisions: renames an auto-named entry colliding with an existing connection", () => {
+  // Provider already has "Key 1" saved; a fresh bulk parse restarts auto-naming
+  // at "Key 1" too. Without renaming, createProviderConnection would upsert
+  // into the existing row instead of inserting.
+  const entries = [{ name: "Key 1" }, { name: "Key 2" }];
+  const out = resolveBulkNameCollisions(entries, ["Key 1"]);
+  assert.deepEqual(
+    out.map((e) => e.name),
+    ["Key 2", "Key 3"]
+  );
+  // Never reuses an existing name.
+  assert.ok(!out.some((e) => e.name === "Key 1"));
+});
+
+test("resolveBulkNameCollisions: renames a custom name colliding with an existing connection", () => {
+  const entries = [{ name: "Prod" }];
+  const out = resolveBulkNameCollisions(entries, ["Prod"]);
+  assert.deepEqual(out.map((e) => e.name), ["Prod 1"]);
+});
+
+test("resolveBulkNameCollisions: dedupes two identical custom names within the same batch", () => {
+  // Two "Prod|apiKey" lines pasted in the same batch previously collided with
+  // EACH OTHER (custom names get no auto-index), so the second entry would
+  // upsert into the first instead of inserting a second connection.
+  const entries = [{ name: "Prod" }, { name: "Prod" }];
+  const out = resolveBulkNameCollisions(entries, []);
+  const names = out.map((e) => e.name);
+  assert.equal(new Set(names).size, names.length);
+  assert.deepEqual(names, ["Prod", "Prod 1"]);
+});
+
+test("resolveBulkNameCollisions: preserves non-name fields on renamed entries", () => {
+  const entries = [{ name: "Key 1", apiKey: "sk-new" }];
+  const out = resolveBulkNameCollisions(entries, ["Key 1"]);
+  assert.equal(out[0].apiKey, "sk-new");
+  assert.equal(out[0].name, "Key 2");
+});
+
+test("resolveBulkNameCollisions: name comparison is case-insensitive", () => {
+  const entries = [{ name: "key 1" }];
+  const out = resolveBulkNameCollisions(entries, ["KEY 1"]);
+  assert.equal(out[0].name, "key 2");
+});
+
+test("resolveBulkNameCollisions: tolerates non-array existingNames", () => {
+  const entries = [{ name: "Key 1" }];
+  const out = resolveBulkNameCollisions(entries, null);
+  assert.equal(out[0].name, "Key 1");
+});