From ede77d1df0f134070353ab8e962016ce4a930a7b Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Wed, 8 Jul 2026 00:33:56 -0300 Subject: [PATCH] fix(api): stop POST /api/keys hanging on the fire-and-forget Cloud sync (#6570) (#6624) cloudEnabled defaults to true in settings.ts::getSettings() for any install with no persisted settings row (every fresh install), so the create-key handler's unconditional `await syncKeysToCloudIfEnabled()` always attempted a real outbound fetch() to CLOUD_URL via syncToCloud(). When that endpoint is unset/unreachable/slow, the HTTP response blocked until the request settled or timed out (20-90s+), unlike sibling routes (regenerate, /api/combos) that never touch this side effect. syncKeysToCloudIfEnabled() is now dispatched fire-and-forget instead of awaited; its internal try/catch already logs failures, so cloud sync still runs in the background without blocking the response. --- CHANGELOG.md | 1 + src/app/api/keys/route.ts | 11 +- tests/integration/api-keys.test.ts | 14 ++- .../unit/api-keys-create-no-hang-6570.test.ts | 105 ++++++++++++++++++ 4 files changed, 127 insertions(+), 4 deletions(-) create mode 100644 tests/unit/api-keys-create-no-hang-6570.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 746f98aab4..bc3623c388 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral - **fix(providers):** the **Auggie (Augment CLI)** executor no longer fails on Windows with `spawn EINVAL` ([#6304](https://github.com/diegosouzapw/OmniRoute/issues/6304)) — the global-npm install exposes `auggie` as a `.cmd` shim, which Node's `child_process.spawn` cannot launch on win32 without `shell: true`. Both spawn sites (streaming + the `auggie --version` test) now go through a shared `buildAuggieSpawnOptions()` that sets `shell: process.platform === "win32"`; the argv (built by `buildAuggieArgs()` with a registry-validated `model` and a trailing `--` end-of-options marker) is unchanged, so the argument-injection surface stays closed on non-Windows. Regression guard: `tests/unit/auggie-win32-spawn-6304.test.ts`. - **fix(api):** the dashboard **"Test model"** action is now a clean connection test ([#6240](https://github.com/diegosouzapw/OmniRoute/issues/6240)) — `modelTestRunner` sent its probe request without an explicit compression override, so whenever the operator's global `compression.enabled` flag was on the test call inherited compression (and any Output-Styles system prompt), polluting the result. The internal test requests now send `X-OmniRoute-Compression: off`, and `chatCore` honors an explicit `off` header even when `compression.enabled` is globally true. Regression guards: `tests/unit/model-test-runner-compression-off-6240.test.ts`, `tests/integration/test-model-compression-off-6240.test.ts`. - **fix(startup):** an update/restart could crash the whole server at boot with `TypeError: Cannot create property 'message' on string 'Database closed'`, masking the real failure and 500-ing every request until manually restarted ([#6560](https://github.com/diegosouzapw/OmniRoute/issues/6560), plausibly the root cause of #6594's post-upgrade 500) — `driverFactory.ts::preInitSqlJs()` cached its sql.js WASM adapter per file path in a `globalThis`-backed map for idempotency, but never checked whether the cached adapter had since been closed (e.g. by `gracefulShutdown`/`resetDbInstance` racing a reload); reusing that dead handle made the very next query throw sql.js's own bare string `"Database closed"` (not an `Error`) straight out of `instrumentation-node.ts`'s previously-unguarded `ensureDbInitialized()` call, and Next.js's internal `registerInstrumentation()` wrapper unconditionally does `err.message = ...` on whatever `register()` rejects with — assigning `.message` on a primitive string throws in strict mode, so the secondary `TypeError` is what actually crashed the process. Fixed in two parts: `preInitSqlJs()` now evicts a closed cached adapter and creates a fresh one instead of returning it; a new `ensureDbReadyForBoot()` wraps the DB-init call, normalizes any non-Error throw via `normalizeBootError()`, and retries once specifically for a transient "database closed" message (now succeeding against the fresh adapter) before re-throwing anything else as a real `Error`. Regression guard: `tests/unit/instrumentation-database-closed-6560.test.ts`. +- **fix(api):** `POST /api/keys` no longer hangs 20–90+ seconds on a fresh install, even with valid auth ([#6570](https://github.com/diegosouzapw/OmniRoute/issues/6570)) — `cloudEnabled` defaults to `true` in `src/lib/db/settings.ts::getSettings()` on any install with no persisted settings row (i.e. every fresh install), so the create-key handler's unconditional `await syncKeysToCloudIfEnabled()` always attempted a real outbound `fetch()` to `CLOUD_URL` via `syncToCloud()`; when that endpoint is unset/unreachable/slow, the HTTP response blocked until the request settled or timed out — unlike sibling routes (`POST /api/keys/:id/regenerate`, `GET /api/combos`), which never touch this side effect at all. `src/app/api/keys/route.ts` now dispatches `syncKeysToCloudIfEnabled()` fire-and-forget (`void`) instead of awaiting it; its internal try/catch already logs failures, so cloud sync still runs, it just no longer blocks the response. Regression guard: `tests/unit/api-keys-create-no-hang-6570.test.ts` (asserts the route resolves in well under 2s even when the Cloud-sync `fetch()` is stubbed to never settle) + updated `tests/integration/api-keys.test.ts` (the pre-existing "triggers cloud sync"/"still succeeds when cloud sync fails" tests, which previously hung indefinitely on this exact path, now await the fire-and-forget sync tick before asserting). ### 📝 Maintenance diff --git a/src/app/api/keys/route.ts b/src/app/api/keys/route.ts index b4611943f6..a28f3741c3 100644 --- a/src/app/api/keys/route.ts +++ b/src/app/api/keys/route.ts @@ -93,8 +93,15 @@ export async function POST(request) { }); } - // Auto sync to Cloud if enabled - await syncKeysToCloudIfEnabled(); + // Auto sync to Cloud if enabled — fire-and-forget. Cloud sync is a + // background side-effect, not part of the key-creation contract, and it + // performs an outbound network call. Awaiting it here blocked the HTTP + // response on a slow/unreachable Cloud endpoint (e.g. a fresh/offline + // install with a misconfigured or unreachable CLOUD_URL): the request + // would hang until the fetch settled or timed out (#6570). Errors inside + // syncKeysToCloudIfEnabled() are already caught and logged internally, so + // this is safe to leave unawaited. + void syncKeysToCloudIfEnabled(); return NextResponse.json( { diff --git a/tests/integration/api-keys.test.ts b/tests/integration/api-keys.test.ts index cf6dd31698..1c4b6337d2 100644 --- a/tests/integration/api-keys.test.ts +++ b/tests/integration/api-keys.test.ts @@ -278,10 +278,15 @@ test("POST /api/keys triggers cloud sync when cloud mode is enabled", async () = }) ); const body = (await response.json()) as any; - const syncPayload = JSON.parse(calls[0].options.body); assert.equal(response.status, 201); assert.equal(body.name, "Cloud Synced Key"); + + // #6570: cloud sync is fire-and-forget so it no longer blocks the + // response — give the background task a moment to run before asserting + // it happened. + await new Promise((resolve) => setTimeout(resolve, 50)); + const syncPayload = JSON.parse(calls[0].options.body); assert.equal(calls.length, 1); assert.match(String(calls[0].url), /^http:\/\/cloud\.example\/sync\//); assert.ok(Array.isArray(syncPayload.providers)); @@ -348,8 +353,13 @@ test("POST /api/keys still succeeds when cloud sync fails after creation", async assert.equal(response.status, 201); assert.equal(body.name, "Cloud Failure Tolerated"); - assert.equal(syncAttempts, 1); assert.equal(stored?.name, "Cloud Failure Tolerated"); + + // #6570: cloud sync is fire-and-forget so it no longer blocks the + // response — give the background task a moment to run before asserting + // the (failed) sync attempt happened and was tolerated. + await new Promise((resolve) => setTimeout(resolve, 50)); + assert.equal(syncAttempts, 1); } finally { globalThis.fetch = originalFetch; } diff --git a/tests/unit/api-keys-create-no-hang-6570.test.ts b/tests/unit/api-keys-create-no-hang-6570.test.ts new file mode 100644 index 0000000000..7ba196ea48 --- /dev/null +++ b/tests/unit/api-keys-create-no-hang-6570.test.ts @@ -0,0 +1,105 @@ +// Regression test for #6570 — POST /api/keys hung 20-90s+ on a fresh install. +// +// Root cause: `cloudEnabled` defaults to `true` (src/lib/db/settings.ts) for any +// install that has never persisted a settings row (i.e. a fresh install). The +// POST /api/keys handler unconditionally `await`ed `syncKeysToCloudIfEnabled()` +// after creating the key, which — when cloud sync is enabled — calls +// `syncToCloud()` and performs a real outbound `fetch()` to `CLOUD_URL`. On a +// fresh/offline install (or any environment where the Cloud endpoint is slow or +// unreachable), that fetch call blocks the HTTP response until it resolves or +// times out, unlike sibling routes such as `POST /api/keys/:id/regenerate` and +// `GET /api/combos`, which never touch this cloud-sync side effect at all. +// +// This test stubs `globalThis.fetch` to hang forever (never settles) and +// asserts the route still responds promptly — i.e. the handler must not await +// the cloud-sync side effect in the request path. +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 { makeManagementSessionRequest } from "../helpers/managementSession.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-api-keys-nohang-6570-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "test-api-key-secret-6570"; +process.env.CLOUD_URL = "http://cloud.example"; + +const core = await import("../../src/lib/db/core.ts"); +const apiKeysDb = await import("../../src/lib/db/apiKeys.ts"); +const localDb = await import("../../src/lib/localDb.ts"); +const listRoute = await import("../../src/app/api/keys/route.ts"); + +async function resetStorage() { + delete process.env.INITIAL_PASSWORD; + core.resetDbInstance(); + apiKeysDb.resetApiKeyState(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +async function enableManagementAuth() { + process.env.INITIAL_PASSWORD = "bootstrap-password"; + await localDb.updateSettings({ requireLogin: true, password: "" }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + await resetStorage(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + core.resetDbInstance(); +}); + +test("POST /api/keys responds promptly even when the Cloud-sync fetch hangs forever (#6570)", async () => { + await enableManagementAuth(); + // cloudEnabled defaults to true on a fresh install (no settings row yet) — + // simulate that "fresh install" condition explicitly for clarity/documentation. + const settings = await localDb.getSettings(); + assert.equal(settings.cloudEnabled, true, "cloudEnabled should default to true pre-fix"); + + const originalFetch = globalThis.fetch; + // Simulate a slow/unreachable Cloud endpoint: a fetch() that never settles. + let fetchWasCalled = false; + globalThis.fetch = (() => { + fetchWasCalled = true; + return new Promise(() => { + /* never resolves — simulates a hung/unreachable outbound network call */ + }); + }) as typeof fetch; + + try { + const start = Date.now(); + const response = await Promise.race([ + listRoute.POST( + await makeManagementSessionRequest("http://localhost/api/keys", { + method: "POST", + body: { name: "Fresh Install Key" }, + }) + ), + new Promise((_, reject) => + setTimeout(() => reject(new Error("POST /api/keys did not respond within 2s")), 2000) + ), + ]); + const elapsedMs = Date.now() - start; + + const body = (await (response as Response).json()) as { key: string }; + assert.equal((response as Response).status, 201); + assert.match(body.key, /^sk-[a-z0-9-]+/i); + assert.ok( + elapsedMs < 2000, + `expected POST /api/keys to resolve well under 2s even with a hung Cloud fetch, took ${elapsedMs}ms` + ); + + // The cloud-sync side effect is fire-and-forget: give the still-pending + // background task a chance to start (it never resolves, by design) before + // asserting it was actually invoked — this proves the fix didn't just + // remove the sync call outright. + await new Promise((resolve) => setTimeout(resolve, 50)); + assert.ok(fetchWasCalled, "expected the stubbed Cloud-sync fetch to have been invoked"); + } finally { + globalThis.fetch = originalFetch; + } +});