From a2ebc343d474a818d810523b7208a113d5feff81 Mon Sep 17 00:00:00 2001 From: Paijo <14921983+oyi77@users.noreply.github.com> Date: Sat, 18 Jul 2026 21:33:28 +0700 Subject: [PATCH] fix: add re-entrancy guard to token health check sweep (#6917) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add re-entrancy guard to token health check sweep Adds an in-flight guard to prevent overlapping sweep() executions. Uses global state sweeping flag that is set before the first await and cleared in a finally block. Subsequent calls while sweeping return early with a debug log line. Test coverage: - skips when a previous sweep is still in flight - resets sweeping flag after normal completion - resets sweeping flag on empty connections * fix(test): move sweep re-entrancy test to node:test, wire CI correctly tests/unit/token-health-check-sweep.test.ts used vitest syntax while living directly under tests/unit/, which is exactly the glob `npm run test:unit` (node's native runner) scans — running it there threw "Vitest mocker was not initialized" and failed the file outright. Separately, the vitest.config.ts include-array edit didn't wire the test into any CI-blocking script either: `npm run test:vitest` runs vitest.mcp.config.ts (a different config, not this path), and test:vitest:ui is scoped to tests/unit/ui only — so the 3 tests never ran in CI at all while node's runner actively failed on the file. Rewrite the test to node:test, matching the tests/unit/apikey-connection-health-check.test.ts / tests/unit/token-health-check.test.ts convention (real temp-dir SQLite DB rather than vi.mock, since mock.module() is unavailable in this tsx/ESM + Node native test-runner setup). The re-entrancy scenario now drives the real, unmocked sweep() with real OAuth connections (healthCheckInterval: 0 keeps checkConnection() a fast no-op) and asserts on wall-clock elapsed time + the shared sweeping flag instead of a mocked call count. Verified this fails without the guard (909ms, ~3x the stagger) and passes with it restored. Remove the now-unused vitest.config.ts include entry since the test no longer needs it. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * refactor(health): compact sweep guard + restore one-line stagger delay (file-size cap) Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: oyi77 Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- src/lib/tokenHealthCheck.ts | 15 +- tests/unit/token-health-check-sweep.test.ts | 146 ++++++++++++++++++++ 2 files changed, 157 insertions(+), 4 deletions(-) create mode 100644 tests/unit/token-health-check-sweep.test.ts diff --git a/src/lib/tokenHealthCheck.ts b/src/lib/tokenHealthCheck.ts index b21b91005c..8fcfe5d7a6 100644 --- a/src/lib/tokenHealthCheck.ts +++ b/src/lib/tokenHealthCheck.ts @@ -277,12 +277,12 @@ export function clearHealthCheckLogCache() { declare global { var __omnirouteTokenHC: - { initialized: boolean; interval: ReturnType | null } | undefined; + | { initialized: boolean; interval: ReturnType | null; sweeping: boolean } + | undefined; } - function getHCState() { if (!globalThis.__omnirouteTokenHC) { - globalThis.__omnirouteTokenHC = { initialized: false, interval: null }; + globalThis.__omnirouteTokenHC = { initialized: false, interval: null, sweeping: false }; } return globalThis.__omnirouteTokenHC; } @@ -322,7 +322,12 @@ export function stopTokenHealthCheck() { } // ── Core sweep ─────────────────────────────────────────────────────────────── -async function sweep() { +export async function sweep() { + const state = getHCState(); + if (state.sweeping) { + return log(`${LOG_PREFIX} Sweep skipped — previous sweep still in progress`); + } + state.sweeping = true; try { const connections = await getProviderConnections({ authType: "oauth" }); @@ -346,6 +351,8 @@ async function sweep() { } } catch (err) { logError(`${LOG_PREFIX} Sweep error:`, err.message); + } finally { + state.sweeping = false; } } diff --git a/tests/unit/token-health-check-sweep.test.ts b/tests/unit/token-health-check-sweep.test.ts new file mode 100644 index 0000000000..841d6d250b --- /dev/null +++ b/tests/unit/token-health-check-sweep.test.ts @@ -0,0 +1,146 @@ +/** + * Regression test: sweep() re-entrancy guard. + * + * Bug: initTokenHealthCheck() schedules sweep() on both a one-shot startup + * timer and a recurring setInterval(sweep, TICK_MS), both fire-and-forget + * (unawaited). If a sweep is still processing connections (slowed down by + * many OAuth connections plus the inter-connection stagger delay) when the + * next tick fires, two sweeps could run concurrently — doubling upstream + * refresh traffic and racing DB writes for the same connections. + * + * Fix: a `sweeping` boolean guard on the shared health-check global state is + * set synchronously before the first `await` in sweep() and cleared in a + * `finally` block, so a sweep() call that starts while another is already in + * flight returns immediately instead of re-entering the loop. + * + * This test drives the REAL (unmocked) sweep() against a temp SQLite DB — + * matching the tests/unit/apikey-connection-health-check.test.ts and + * tests/unit/token-health-check.test.ts convention — rather than mocking + * @/lib/localDb, since mock.module() is unavailable in this tsx/ESM + Node + * native test-runner setup (see tests/unit/proxyfetch-undici-retry.test.ts + * and tests/unit/rule12-error-sanitization-sweep.test.ts). Connections are + * created with healthCheckInterval: 0 so checkConnection() is a fast no-op + * (returns before any network/DB-write work), letting the test control + * timing purely via the inter-connection HEALTHCHECK_STAGGER_MS delay. + */ +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-sweep-reentrancy-")); +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 { sweep } = await import("../../src/lib/tokenHealthCheck.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(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +/** Read the shared health-check global state's `sweeping` flag. */ +function isSweeping(): boolean { + const hc = (globalThis as Record).__omnirouteTokenHC as + | { sweeping?: boolean } + | undefined; + return hc?.sweeping ?? false; +} + +/** Create `count` distinct OAuth connections whose checkConnection() is a fast no-op. */ +async function createNoOpOauthConnections(count: number, namePrefix: string) { + for (let i = 0; i < count; i++) { + await providersDb.createProviderConnection({ + provider: "anthropic", + name: `${namePrefix}-${i}`, + authType: "oauth", + isActive: true, + healthCheckInterval: 0, // disabled — checkConnection() returns immediately (no network/db-write) + }); + } +} + +test("sweep() skips re-entrant calls while a previous sweep is still in flight", async () => { + await resetStorage(); + process.env.HEALTHCHECK_STAGGER_MS = "300"; + await createNoOpOauthConnections(4, "reentrancy"); + + const seeded = await providersDb.getProviderConnections({ authType: "oauth" }); + assert.equal(seeded.length, 4, "precondition: 4 oauth connections exist"); + assert.equal(isSweeping(), false, "precondition: no sweep in flight yet"); + + const first = sweep(); + // sweep() sets state.sweeping = true synchronously before its first + // `await`, so this is already true the instant sweep() returns control to + // us — no microtask boundary needed to observe it. + assert.equal(isSweeping(), true, "first sweep should mark sweeping=true immediately"); + + const start = Date.now(); + await sweep(); // second, concurrent call — must be skipped by the guard + const elapsedMs = Date.now() - start; + + // With 4 connections and a 300ms inter-connection stagger, a REAL second + // sweep would take >= 300ms just to clear a single stagger gap (and up to + // ~900ms for all three gaps). The guard must make this call return near- + // instantly instead of re-entering the loop. + assert.ok( + elapsedMs < 250, + `re-entrant sweep() call should short-circuit almost instantly, took ${elapsedMs}ms` + ); + + // The first sweep is still running (its stagger delays haven't elapsed) — + // proves the second call didn't reset or otherwise interfere with the + // first sweep's in-flight state. + assert.equal(isSweeping(), true, "first sweep should still be in flight after the skipped call"); + + await first; + assert.equal(isSweeping(), false, "sweeping flag clears once the in-flight sweep finishes"); +}); + +test("sweep() resets the sweeping flag after a normal completion, allowing the next sweep to run", async () => { + await resetStorage(); + process.env.HEALTHCHECK_STAGGER_MS = "0"; + await createNoOpOauthConnections(2, "sequential"); + + await sweep(); + assert.equal(isSweeping(), false, "flag resets after first sweep completes"); + + // A second, fully sequential call must not be skipped by leftover state — + // it should run and complete normally rather than hang or short-circuit. + await sweep(); + assert.equal(isSweeping(), false, "flag resets after second sweep completes too"); +}); + +test("sweep() resets the sweeping flag even when there are no connections to process", async () => { + await resetStorage(); + process.env.HEALTHCHECK_STAGGER_MS = "0"; + // No connections created — sweep() takes the early `connections.length === 0` return. + + await sweep(); + + assert.equal(isSweeping(), false, "sweeping flag must be false after an empty sweep"); +});