mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-01 04:42:10 +03:00
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.
This commit is contained in:
committed by
GitHub
parent
48e902c9d0
commit
ede77d1df0
@@ -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
|
||||
|
||||
|
||||
@@ -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(
|
||||
{
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
105
tests/unit/api-keys-create-no-hang-6570.test.ts
Normal file
105
tests/unit/api-keys-create-no-hang-6570.test.ts
Normal file
@@ -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;
|
||||
}
|
||||
});
|
||||
Reference in New Issue
Block a user