mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-18 04:42:30 +03:00
fix(api): stop DISABLE_SQLITE_AUTO_BACKUP from turning off Redis rate limiting (#13329)
A backup flag was being used as a proxy for test mode, so `DISABLE_SQLITE_AUTO_BACKUP` also disabled Redis rate limiting — two unrelated concerns riding one variable. Probe on your head: 3/3 + 13/13 pass across the new test and the existing rate-limiter suite. Thanks, @datrixlab — catching that the existing rate-limiter tests still pass is what shows this untangled the two without changing the intended behavior of either. **Batch validation** — boarded with the other 10 PRs of your batch into one worktree cut from `release/v3.8.51`; every PR verified as an ancestor of the combined HEAD before validating. - Focused tests across all 11 PRs: **104/104 pass** on the combined tree. - Gates on the combined tree: `check-changelog-integrity` PASS, `check-complexity` PASS, `check-cognitive-complexity` PASS, `typecheck:core` PASS, `check:open-sse-typecheck` PASS. - `check-file-size` is red, but reproduces with byte-identical line counts on the pure `release/v3.8.51` tip (`open-sse/handlers/imageGeneration.ts` 3304, `open-sse/services/combo/roundRobinCombo.ts` 1221, `open-sse/utils/stream.ts` 3115). Inherited base-red, nothing added by this batch — it is also why this PR's "Fast Quality Gates" check was red.
This commit is contained in:
1
changelog.d/fixes/13329-rate-limiter-backup-flag.md
Normal file
1
changelog.d/fixes/13329-rate-limiter-backup-flag.md
Normal file
@@ -0,0 +1 @@
|
||||
- **fix(api):** Setting `DISABLE_SQLITE_AUTO_BACKUP=true` no longer makes the API-key rate limiter and auth cache skip Redis, which let every replica enforce the full per-key limit on its own ([#13329](https://github.com/diegosouzapw/OmniRoute/pull/13329))
|
||||
@@ -272,11 +272,7 @@ function isConfiguredEnvApiKey(key: string): boolean {
|
||||
}
|
||||
|
||||
function isRedisAuthCacheEnabled(): boolean {
|
||||
return (
|
||||
process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE !== "1" &&
|
||||
process.env.NODE_ENV !== "test" &&
|
||||
process.env.DISABLE_SQLITE_AUTO_BACKUP !== "true"
|
||||
);
|
||||
return process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE !== "1" && process.env.NODE_ENV !== "test";
|
||||
}
|
||||
|
||||
async function deleteRedisAuthCacheEntry(keyHash: unknown): Promise<void> {
|
||||
|
||||
@@ -230,10 +230,9 @@ export async function checkRateLimit(
|
||||
if (!rules || rules.length === 0) return { allowed: true };
|
||||
|
||||
// ── In-memory mock for unit tests ──
|
||||
const isTestMode =
|
||||
explicitTestMode ||
|
||||
process.env.NODE_ENV === "test" ||
|
||||
process.env.DISABLE_SQLITE_AUTO_BACKUP === "true";
|
||||
// Not DISABLE_SQLITE_AUTO_BACKUP: that is a production setting (backups managed
|
||||
// externally), and treating it as test mode skipped Redis on every replica.
|
||||
const isTestMode = explicitTestMode || process.env.NODE_ENV === "test";
|
||||
|
||||
if (isTestMode) {
|
||||
return checkInMemoryRateLimit(TEST_MEMORY_STORE, keyId, rules);
|
||||
|
||||
71
tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts
Normal file
71
tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts
Normal file
@@ -0,0 +1,71 @@
|
||||
/**
|
||||
* DISABLE_SQLITE_AUTO_BACKUP is a production setting (.env.example: "Set true only when
|
||||
* those backups are managed externally"), but checkRateLimit() and the Redis auth cache
|
||||
* also read it as "running under tests". A deployment with REDIS_URL and externally
|
||||
* managed backups therefore rate-limited every key in process memory, so each replica
|
||||
* enforced the full per-key limit on its own.
|
||||
*
|
||||
* The REDIS_URL below points at a closed port. Taking the Redis path shows up as the
|
||||
* limiter's fail-open { allowed: true }; the in-memory test store would reject the
|
||||
* second request against a limit of 1.
|
||||
*/
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import { pathToFileURL } from "node:url";
|
||||
|
||||
const ENV_KEYS = ["REDIS_URL", "NODE_ENV", "DISABLE_SQLITE_AUTO_BACKUP"] as const;
|
||||
|
||||
async function twoRequestsAgainstLimitOne(env: Partial<Record<(typeof ENV_KEYS)[number], string>>) {
|
||||
const saved = Object.fromEntries(ENV_KEYS.map((key) => [key, process.env[key]]));
|
||||
for (const key of ENV_KEYS) {
|
||||
if (env[key] === undefined) delete process.env[key];
|
||||
else process.env[key] = env[key];
|
||||
}
|
||||
const originalConsoleError = console.error;
|
||||
console.error = () => {};
|
||||
const modulePath = path.join(process.cwd(), "src/shared/utils/rateLimiter.ts");
|
||||
const rateLimiter = await import(`${pathToFileURL(modulePath).href}?case=${Math.random()}`);
|
||||
try {
|
||||
const rules = [{ limit: 1, window: 60 }];
|
||||
return [
|
||||
await rateLimiter.checkRateLimit("key-1", rules),
|
||||
await rateLimiter.checkRateLimit("key-1", rules),
|
||||
];
|
||||
} finally {
|
||||
if (rateLimiter.isRedisConfigured()) {
|
||||
(await rateLimiter.getRedisClient()).disconnect();
|
||||
}
|
||||
console.error = originalConsoleError;
|
||||
for (const key of ENV_KEYS) {
|
||||
if (saved[key] === undefined) delete process.env[key];
|
||||
else process.env[key] = saved[key];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
test("DISABLE_SQLITE_AUTO_BACKUP=true does not move a Redis deployment to the in-memory store", async () => {
|
||||
const results = await twoRequestsAgainstLimitOne({
|
||||
REDIS_URL: "redis://127.0.0.1:1",
|
||||
NODE_ENV: "production",
|
||||
DISABLE_SQLITE_AUTO_BACKUP: "true",
|
||||
});
|
||||
assert.deepEqual(results, [{ allowed: true }, { allowed: true }]);
|
||||
});
|
||||
|
||||
test("NODE_ENV=test still keeps the limiter in memory even with REDIS_URL set", async () => {
|
||||
const results = await twoRequestsAgainstLimitOne({
|
||||
REDIS_URL: "redis://127.0.0.1:1",
|
||||
NODE_ENV: "test",
|
||||
});
|
||||
assert.deepEqual(results, [{ allowed: true }, { allowed: false, failedWindow: 60 }]);
|
||||
});
|
||||
|
||||
test("the Redis auth cache is not switched off by DISABLE_SQLITE_AUTO_BACKUP", () => {
|
||||
const source = fs.readFileSync(path.join(process.cwd(), "src/lib/db/apiKeys.ts"), "utf8");
|
||||
const gate = source.match(/function isRedisAuthCacheEnabled\(\)[^{]*\{([\s\S]*?)\n\}/);
|
||||
assert.ok(gate, "isRedisAuthCacheEnabled() not found");
|
||||
assert.doesNotMatch(gate[1], /DISABLE_SQLITE_AUTO_BACKUP/);
|
||||
assert.match(gate[1], /NODE_ENV !== "test"/);
|
||||
});
|
||||
Reference in New Issue
Block a user