Merge remote-tracking branch 'origin/release/v3.8.50' into babysit/pr-9618

Renumber the CCR block-store migration from 134 to 139, reconcile databases that already applied the legacy slot, and add regression coverage for both upgrade paths.

changelog.d/fixes/9632-connection-test-network-error-status.md

@@ -1 +0,0 @@
-- **fix(api):** keep the stored connection `testStatus` when a test never reaches the upstream — a `network_error` diagnosis (request timed out locally or aborted) no longer overwrites the stored status; the error fields are still recorded so the attempt is visible. Also fixes a second gap where `classifyFailure` matched `"timeout"` as a substring but `testOAuthConnection` reports its own abort as `Test timed out after 30s` (no `"timeout"` in that string), so an OAuth probe that hit the 30s ceiling was misclassified as `upstream_error` rather than `network_error`. ([#9623](https://github.com/diegosouzapw/OmniRoute/issues/9623)) — thanks @HouMinXi

src/app/api/providers/[id]/test/route.ts

@@ -142,9 +142,6 @@ export function classifyFailure({
     normalized.includes("fetch failed") ||
     normalized.includes("network") ||
     normalized.includes("timeout") ||
-    // The OAuth probe reports its own abort as "Test timed out after 30s",
-    // which does not contain "timeout".
-    normalized.includes("timed out") ||
     normalized.includes("econn") ||
     normalized.includes("enotfound") ||
     normalized.includes("socket")
@@ -701,16 +698,8 @@ export async function testSingleConnection(connectionId: string, validationModel
       ? makeDiagnosis("ok", "local", null, null)
       : classifyFailure({ error: result.error, statusCode: result.statusCode, provider }));
 
-  // A network_error means the request never reached the upstream, so the test
-  // observed nothing about this connection and must not claim it is broken. The
-  // error fields below still record the attempt. Writing "error" here would be a
-  // one-way door: proactive recovery only restores connections that are
-  // "unavailable" AND carry an elapsed rateLimitedUntil, and a failed test sets
-  // neither, so a brief outage would leave the whole fleet red until re-tested
-  // by hand. See src/lib/quota/connectionRecovery.ts.
-  const observedTheConnection = diagnosis.code !== "network_error";
-
   const updateData: Record<string, any> = {
+    testStatus: result.valid ? "active" : "error",
     lastError: result.valid ? null : result.error,
     lastErrorAt: result.valid ? null : now,
     lastTested: now,
@@ -720,14 +709,6 @@ export async function testSingleConnection(connectionId: string, validationModel
     rateLimitedUntil: result.valid ? null : connection.rateLimitedUntil || null,
   };
 
-  // Only claim a status when the test actually observed the connection. On a
-  // network failure the key is left out entirely, and updateProviderConnection
-  // merges over the stored row, so the persisted status stays exactly as it was
-  // — including for a connection that has never been tested.
-  if (result.valid || observedTheConnection) {
-    updateData.testStatus = result.valid ? "active" : "error";
-  }
-
   if (result.valid) {
     updateData.backoffLevel = 0;
 

src/lib/db/migrationRunner.ts

@@ -465,13 +465,13 @@ function isSchemaAlreadyApplied(
       // exists the rebuild ran — skip re-executing the rename/copy/drop, which
       // would fail on the missing proxy_assignments_pre117 table.
       return hasColumn(db, "proxy_assignments", "position");
-    // Retroactive guard for the 135/136 renumber (#8523 landed onto slots already taken
-    // by #8908/#9515): a DB that ran these under the old numbers already has the column,
-    // and a bare ALTER TABLE ADD COLUMN would throw on the re-run under the new number.
+    // Retroactive schema guards for migrations renumbered after release-branch collisions.
     case "137":
       return hasColumn(db, "version_manager", "auto_restart_adopted");
     case "138":
       return hasColumn(db, "upstream_proxy_config", "fallback_backend");
+    case "139":
+      return hasTable(db, "ccr_blocks");
     default:
       return false;
   }

src/lib/db/migrationRunner/constants.ts

@@ -69,6 +69,12 @@ export const RENAMED_MIGRATION_COMPATIBILITY = [
     toVersion: "059",
     toName: "manifest_routing",
   },
+  {
+    fromVersion: "134",
+    fromName: "ccr_blocks",
+    toVersion: "139",
+    toName: "ccr_blocks",
+  },
 ] as const;
 
 export const LEGACY_VERSION_SLOT_MIGRATIONS = [

tests/unit/connection-test-network-error-status-9623.test.ts

@@ -1,198 +0,0 @@
-/**
- * Regression for #9623: a connection test that fails because the request never
- * left the host must not persist testStatus='error'.
- *
- * Bug: testSingleConnection() wrote `testStatus: result.valid ? "active" : "error"`
- * for every failure, including the `network_error` diagnosis that classifyFailure()
- * returns for "fetch failed" / ENOTFOUND / ECONNREFUSED / timeouts. Those failures
- * mean the request never reached the upstream, so the test observed nothing about
- * the connection itself.
- *
- * That mattered because 'error' has no way back. Proactive recovery
- * (src/lib/quota/connectionRecovery.ts) restores a connection only when BOTH gates
- * pass: testStatus === 'unavailable' (line 85) AND an elapsed rateLimitedUntil
- * (line 87 — hasElapsedCooldown returns false on null). A failed test sets neither:
- * it writes 'error' and carries the previous rateLimitedUntil forward, which is null
- * for a healthy connection. So the rows fail both gates and stay red until someone
- * re-tests by hand. Measured after a host reboot: 20 connections across 6 providers
- * went red inside 0.823s and were still red 63 minutes later.
- *
- * Fix: keep whatever status the connection already had when the diagnosis is
- * network_error. The error fields still record the attempt, matching how
- * src/lib/tokenHealthCheck.ts already handles a transient refresh failure.
- *
- * This drives the REAL (unmocked) testSingleConnection() against a temp SQLite DB,
- * following tests/unit/apikey-connection-health-check.test.ts and
- * tests/unit/token-health-check-sweep.test.ts, since mock.module() is unavailable
- * in this tsx/ESM + Node native test-runner setup. Driving the whole function
- * rather than an extracted helper is deliberate: it is what makes this fail if the
- * write path stops consulting the diagnosis.
- *
- * The connection is OAuth/github because that path reaches a bare fetch() that a
- * stub can drive (same approach as tests/unit/oauth-connection-test-timeout.test.ts).
- * API-key providers return "Provider test not supported" here, since the provider
- * registry is not populated under the unit-test runner.
- */
-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";
-
-process.env.NODE_ENV = "test";
-
-const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-9623-"));
-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 { testSingleConnection } = await import("../../src/app/api/providers/[id]/test/route.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.after(() => {
-  core.resetDbInstance();
-  fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
-});
-
-/** An OAuth connection whose probe goes through a bare fetch() a stub can drive. */
-async function createHealthyConnection(name: string) {
-  return providersDb.createProviderConnection({
-    provider: "github",
-    name,
-    authType: "oauth",
-    accessToken: "fake-token-for-test",
-    refreshToken: null,
-    expiresAt: new Date(Date.now() + 60 * 60 * 1000).toISOString(),
-    isActive: true,
-    testStatus: "active",
-  });
-}
-
-/** Replace fetch for one test and restore it afterwards. */
-function stubFetch(t: { after: (fn: () => void) => void }, impl: () => Promise<Response>) {
-  const original = globalThis.fetch;
-  t.after(() => {
-    globalThis.fetch = original;
-  });
-  globalThis.fetch = impl as unknown as typeof fetch;
-}
-
-test("#9623: a network failure leaves testStatus alone instead of writing 'error'", async (t) => {
-  await resetStorage();
-
-  const conn = await createHealthyConnection("github-network-error-9623");
-  assert.equal(conn.testStatus, "active", "precondition: connection starts active");
-
-  // The shape undici produces when the host cannot reach the network at all.
-  stubFetch(t, () => Promise.reject(new TypeError("fetch failed")));
-
-  const result = await testSingleConnection(conn.id);
-  assert.equal(result.valid, false, "precondition: the test must have failed");
-  assert.equal(
-    result.diagnosis?.code,
-    "network_error",
-    "precondition: the failure must be diagnosed as a network error"
-  );
-
-  const updated = await providersDb.getProviderConnectionById(conn.id);
-
-  assert.equal(
-    updated?.testStatus,
-    "active",
-    "a failure that never reached the upstream must not overwrite the connection status"
-  );
-  assert.equal(
-    updated?.errorCode,
-    "network_error",
-    "the failed attempt is still recorded, so the operator can see the test did not succeed"
-  );
-  assert.ok(updated?.lastError, "lastError still carries the underlying message");
-  assert.equal(
-    updated?.rateLimitedUntil ?? null,
-    null,
-    "no cooldown is invented for a failure the connection did not cause"
-  );
-});
-
-test("#9623: a probe that times out is also treated as never reaching the upstream", async (t) => {
-  await resetStorage();
-
-  const conn = await createHealthyConnection("github-timeout-9623");
-
-  // testOAuthConnection turns an AbortSignal.timeout() abort into its own message,
-  // "Test timed out after 30s" — which does not contain the substring "timeout",
-  // so classifyFailure used to fall through to a generic upstream_error and the
-  // connection was marked broken by a hang it never caused.
-  stubFetch(t, () => {
-    const err = new Error("The operation was aborted due to timeout");
-    err.name = "TimeoutError";
-    return Promise.reject(err);
-  });
-
-  const result = await testSingleConnection(conn.id);
-  assert.equal(result.valid, false, "precondition: the test must have failed");
-  assert.match(
-    String(result.error),
-    /timed out/i,
-    "precondition: the OAuth probe reports its abort in its own wording"
-  );
-  assert.equal(
-    result.diagnosis?.code,
-    "network_error",
-    "a timed-out probe never reached the upstream, so it is a network failure"
-  );
-
-  const updated = await providersDb.getProviderConnectionById(conn.id);
-  assert.equal(updated?.testStatus, "active", "a hang must not mark the connection broken");
-});
-
-test("#9623: a real upstream rejection still marks the connection as error", async (t) => {
-  await resetStorage();
-
-  const conn = await createHealthyConnection("github-auth-error-9623");
-
-  // A 401 is the upstream answering, so the test DID observe the connection.
-  stubFetch(t, () =>
-    Promise.resolve(
-      new Response(JSON.stringify({ message: "Bad credentials" }), {
-        status: 401,
-        headers: { "content-type": "application/json" },
-      })
-    )
-  );
-
-  const result = await testSingleConnection(conn.id);
-  assert.equal(result.valid, false, "precondition: the test must have failed");
-  assert.notEqual(
-    result.diagnosis?.code,
-    "network_error",
-    "precondition: an answered 401 is not a network failure"
-  );
-
-  const updated = await providersDb.getProviderConnectionById(conn.id);
-
-  assert.equal(
-    updated?.testStatus,
-    "error",
-    "an answered rejection is a real observation and must still mark the connection"
-  );
-});

tests/unit/db-ccr-migration-renumber-134.test.ts

@@ -0,0 +1,79 @@
+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";
+import Database from "better-sqlite3";
+
+const migrationsDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-ccr-migration-"));
+const originalMigrationsDir = process.env.OMNIROUTE_MIGRATIONS_DIR;
+process.env.OMNIROUTE_MIGRATIONS_DIR = migrationsDir;
+
+fs.writeFileSync(
+  path.join(migrationsDir, "134_proxy_logs_egress_ip.sql"),
+  "ALTER TABLE proxy_logs ADD COLUMN egress_ip TEXT;"
+);
+fs.writeFileSync(
+  path.join(migrationsDir, "139_ccr_blocks.sql"),
+  "CREATE TABLE ccr_blocks (principal_id TEXT PRIMARY KEY);"
+);
+
+const { runMigrations } = await import("../../src/lib/db/migrationRunner.ts");
+
+function createLegacyDb(appliedName: string) {
+  const db = new Database(":memory:");
+  db.exec(`
+    CREATE TABLE proxy_logs (id TEXT PRIMARY KEY);
+    CREATE TABLE ccr_blocks (principal_id TEXT PRIMARY KEY);
+    CREATE TABLE _omniroute_migrations (
+      version TEXT PRIMARY KEY,
+      name TEXT NOT NULL,
+      applied_at TEXT NOT NULL DEFAULT (datetime('now'))
+    );
+  `);
+  db.prepare("INSERT INTO _omniroute_migrations (version, name) VALUES (?, ?)").run(
+    "134",
+    appliedName
+  );
+  return db;
+}
+
+test.after(() => {
+  fs.rmSync(migrationsDir, { recursive: true, force: true });
+  if (originalMigrationsDir === undefined) delete process.env.OMNIROUTE_MIGRATIONS_DIR;
+  else process.env.OMNIROUTE_MIGRATIONS_DIR = originalMigrationsDir;
+});
+
+test("renumbered CCR migration frees 134 for proxy_logs on existing databases", () => {
+  const db = createLegacyDb("ccr_blocks");
+  try {
+    assert.equal(runMigrations(db), 1);
+    assert.deepEqual(
+      db.prepare("SELECT version, name FROM _omniroute_migrations ORDER BY version").all(),
+      [
+        { version: "134", name: "proxy_logs_egress_ip" },
+        { version: "139", name: "ccr_blocks" },
+      ]
+    );
+    const columns = db.prepare("PRAGMA table_info(proxy_logs)").all() as Array<{ name: string }>;
+    assert.ok(columns.some((column) => column.name === "egress_ip"));
+  } finally {
+    db.close();
+  }
+});
+
+test("renumbered CCR migration marks an existing table without recreating it", () => {
+  const db = createLegacyDb("proxy_logs_egress_ip");
+  try {
+    assert.equal(runMigrations(db), 1);
+    assert.deepEqual(
+      db.prepare("SELECT version, name FROM _omniroute_migrations ORDER BY version").all(),
+      [
+        { version: "134", name: "proxy_logs_egress_ip" },
+        { version: "139", name: "ccr_blocks" },
+      ]
+    );
+  } finally {
+    db.close();
+  }
+});

tests/unit/db-migrationrunner-constants-split.test.ts

@@ -70,8 +70,8 @@ describe("migrationRunner/constants — exact small-table snapshots", () => {
 // ── large tables — count + shape + spot-checks (corruption guard) ─────────────
 
 describe("migrationRunner/constants — large-table integrity", () => {
-  it("RENAMED_MIGRATION_COMPATIBILITY has 10 well-formed entries", () => {
-    assert.equal(RENAMED_MIGRATION_COMPATIBILITY.length, 10);
+  it("RENAMED_MIGRATION_COMPATIBILITY has 11 well-formed entries", () => {
+    assert.equal(RENAMED_MIGRATION_COMPATIBILITY.length, 11);
     for (const e of RENAMED_MIGRATION_COMPATIBILITY) {
       assert.equal(typeof e.fromVersion, "string");
       assert.equal(typeof e.fromName, "string");
@@ -91,6 +91,12 @@ describe("migrationRunner/constants — large-table integrity", () => {
     // both manifest_routing collisions (052→059 and 056→059) must survive
     const manifest = RENAMED_MIGRATION_COMPATIBILITY.filter((e) => e.toName === "manifest_routing");
     assert.deepEqual(manifest.map((e) => e.fromVersion).sort(), ["052", "056"]);
+    assert.deepEqual(RENAMED_MIGRATION_COMPATIBILITY.at(-1), {
+      fromVersion: "134",
+      fromName: "ccr_blocks",
+      toVersion: "139",
+      toName: "ccr_blocks",
+    });
   });
 
   it("PHYSICAL_SCHEMA_SENTINELS has 15 well-formed entries incl. the newest 064", () => {