diff --git a/.changeset/fix-188-redis-eviction-policy.md b/.changeset/fix-188-redis-eviction-policy.md new file mode 100644 index 00000000..c6c53332 --- /dev/null +++ b/.changeset/fix-188-redis-eviction-policy.md @@ -0,0 +1,25 @@ +--- +"sql-fs-api": patch +--- + +Require `maxmemory-policy allkeys-lru` on the Redis backing the blob cache, and say so at boot instead of leaving it an unstated assumption. + +**With Redis's default `noeviction`, a full Redis is a permanent outage, not a transient one.** An instance at `maxmemory` refuses every write with `OOM command not allowed`, and blob cache entries carry a 24 h TTL (`REDIS_BLOB_CACHE_TTL_MS`), so nothing ages out fast enough to make room. Someone has to flush keys or raise the limit by hand. The load harness measured that as 97.6% 5xx with no recovery, against a 6 s `CLIENT PAUSE` that recovered the moment the pause lifted. Nothing in the code or the docs asked for a different policy, so every deployment that took the Redis default was one memory spike away from it. With `allkeys-lru` the same pressure evicts cold blobs, which costs a Postgres read. + +**Only `allkeys-lru` and `allkeys-lfu` are accepted.** By default the data role shares an instance with the control role (`REDIS_DATA_URL` falls back to `REDIS_URL`), so whatever policy is set governs the exec-lock leases, version counters and destroy tombstones as well as the cache — which is what rules the other evicting policies out, each for its own reason: + +- `allkeys-lru` / `allkeys-lfu` protect exactly the keys that must survive. A lease renewed every 20 s (`REDIS_EXEC_LOCK_RENEW_MS`) and a version counter touched on every write are the most recently and most frequently used keys in the instance, so a cold blob is always the better candidate. +- `allkeys-random` is refused. It samples uniformly, so a live lease is exactly as likely to be reaped as the cold blob beside it. +- `volatile-*` is refused, but not on recency grounds — the argument that "it can reap a live lease, because the control keys carry a TTL" applies just as well to `allkeys-*`, where those keys are candidates too. The real problem is that it evicts ONLY keys carrying a TTL: once the instance fills with keys that do not (the RW-lock reader ZSETs, anything a later change adds), there is no eviction candidate left and it behaves exactly like `noeviction` — writes refused, no recovery without a human. `allkeys-*` always has a candidate. + +**Documented in `CLAUDE.md` and `README.md` next to `REDIS_URL` / `REDIS_DATA_URL`**, both of which now point at a new "Redis eviction policy" section carrying the `CONFIG SET` line, that reasoning, and the log events. + +**The boot check is a warning, and stays a warning.** `startEvictionPolicyCheck` runs `CONFIG GET maxmemory-policy` against the data client after `listen`. A policy that is neither `allkeys-lru` nor `allkeys-lfu` logs `event:"redis_eviction_policy_unsafe"` at `severity:"critical"` with the policy it found, the command to fix it, and the reason that particular policy was refused — "not allkeys-*" is not a usable reason to give an operator running `allkeys-random`. Managed Redis providers routinely forbid `CONFIG GET` — ElastiCache renames the command, an ACL-restricted user answers `NOPERM` — so that case is recognised on its own and logged as `event:"redis_eviction_policy_unknown"`, `reason:"config_get_denied"`, with a line telling the operator to confirm the setting with their provider. A `CONFIG GET` that fails for any other reason, or answers something unparseable, logs the same event with `reason:"config_get_failed"`. Nothing here can fail startup: the check is never awaited (a slow Redis must not delay `listen`), it never rejects, and it is skipped when no Redis is configured. + +**It is also skipped when the data client carries no data plane.** `REDIS_DATA_URL` falls back to `REDIS_URL`, so with `REDIS_BLOB_CACHE_ENABLED=false` and no path snapshot, the "data" client IS the control instance — and a correctly-configured control-only deployment would have been paged with a `severity:"critical"` line whose remediation makes things worse: switching a control-only Redis to `allkeys-*` makes its exec-lock leases, version counters and destroy tombstones evictable. The check now runs only when the blob cache or the path snapshot is actually enabled. + +**Cost: one extra Redis round trip per boot, and a critical-severity line that most deployments will see on their first restart after upgrading.** That is the intended outcome, but it does mean an operator who alerts on `severity:"critical"` gets paged by an upgrade rather than by an incident, and the honest answer is to fix the policy rather than to filter the event. The check deliberately does not look at `maxmemory` itself: a Redis with no limit set never evicts and never refuses, so the policy is the only thing worth asserting, and an instance sized by its container rather than by `maxmemory` would produce a confusing second warning. It also says nothing about the control-plane Redis when the two are split, because `allkeys-*` is not obviously right there: LRU-evicting a version counter resets it to 1 under a replica that still holds a higher `lastSeenVersion`, which is the reset wrap the H6 TTL design closes. + +Verified against the harness FAULT replica and its disposable Redis on :6380. `CONFIG SET maxmemory-policy noeviction` then restart produced the `redis_eviction_policy_unsafe` line in `fault.log`; `allkeys-lru` then restart produced `{"event":"redis_eviction_policy","policy":"allkeys-lru"}` and zero unsafe lines. The permission-denied path was exercised live too, with a Redis ACL user carrying `+@all -config`: the replica logged `reason:"config_get_denied"` with the `NOPERM` text and still came up `{"status":"ok"}` on `/healthz`. The ACL user was deleted and the original policy (`noeviction`) restored afterwards. `concurrency.mjs` stays all-PASS, exit 0. Twelve unit tests, each checked against a mutation of the code it covers rather than only against the module's absence: accepting every policy as safe, dropping the `OOM`-is-not-a-permission-error carve-out, never classifying a refusal as denied, dropping the RESP3 map reply branch, letting `CONFIG GET` errors escape, warning on a healthy boot, and logging synchronously each kill exactly the tests that claim to cover them. + +**Not verified.** No test drives a real Redis to `maxmemory` under either policy, so the 97.6% figure is quoted from #167's harness run and not re-measured after the #185 role split; the recovery difference between the two policies is a property of eviction and the TTL rather than of this change. The server wiring itself has no unit test, because the bootstrap block only runs when the module is the process entry point, so the only proof that the check is actually called is the harness log, which is what was used. RESP3 map replies are covered by a fake, not by an ioredis client in RESP3 mode. And the check reads the policy once at boot: an operator who changes it afterwards gets no new warning until the next restart. diff --git a/CLAUDE.md b/CLAUDE.md index b188bbd7..dbfad82f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,8 +217,7 @@ const TABLE = Object.assign(Object.create(null) as Record, { | `MAX_CONCURRENT_JS` | No (default: 5) | Max concurrent JavaScript (`js-exec`/`node`) executions across all sessions. QuickJS executions cap at 64MB each. Excess scripts queue FIFO. Note: just-bash currently serializes `js-exec` internally through a single worker, so this cap is an upper bound that may not be binding today. | | `GITHUB_TOKEN` | No | Optional shared GitHub token. When set, exported into `network:true` sandbox shell env as `GITHUB_TOKEN` for `curl` GitHub API calls, plus `GIT_HTTP_USER=x-access-token` and `GIT_HTTP_PASSWORD=` for GitHub-compatible `git` HTTPS auth. This is a deployment-wide identity readable by network-enabled sandbox code; use only with trusted agents. Per-request `env` overrides it. | | `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`, `GIT_COMMITTER_EMAIL` | No | Optional git identity values to export into every sandbox shell env so `git commit` has defaults. Per-request `env` overrides them. | -| `REDIS_URL` | No | Redis connection string. Required for multi-replica deployments. When both `REDIS_URL` and `REDIS_DATA_URL` are absent, distributed exec lock and all Redis caches are disabled — only in-process `session.mutex` protects execution. (`REDIS_DATA_URL` alone still enables the data plane: blob cache / path snapshot.) Carries the **control plane**: exec/RW locks, version counter, session state. | -| `REDIS_DATA_URL` | No (default: `REDIS_URL`) | Connection string for the **data plane** (blob cache, path snapshot). #167: when both roles are opened they get separate ioredis connections even when this resolves to the same URL, because ioredis pipelines over one socket and a queue of multi-MiB blob `SET`s head-of-line blocks the latency-critical `INCR`/`EVAL` behind them (measured: `INCR` timed out at 2043 ms on the shared connection vs 44 ms on a separate one). A lock-only deployment (blob cache off, snapshot off) opens just the control connection. Set it to a different Redis only when you also want physical separation. | +| `REDIS_DATA_URL` | No (default: `REDIS_URL`) | Connection string for the **data plane** (blob cache, path snapshot). #167: when both roles are opened they get separate ioredis connections even when this resolves to the same URL, because ioredis pipelines over one socket and a queue of multi-MiB blob `SET`s head-of-line blocks the latency-critical `INCR`/`EVAL` behind them (measured: `INCR` timed out at 2043 ms on the shared connection vs 44 ms on a separate one). A lock-only deployment (blob cache off, snapshot off) opens just the control connection. Set it to a different Redis only when you also want physical separation. This instance carries the blob cache, so it is the one that must run `maxmemory-policy allkeys-lru` (or `allkeys-lfu`) — see **Redis eviction policy** below. | | `REDIS_EXEC_LOCK_LEASE_MS` | No (default: 60000) | Distributed exec lock lease duration (ms). Lock auto-expires if the holder dies. Must be > `REDIS_EXEC_LOCK_RENEW_MS`. | | `REDIS_EXEC_LOCK_RENEW_MS` | No (default: 20000) | Heartbeat interval for exec lock renewal (ms). Must be strictly less than `REDIS_EXEC_LOCK_LEASE_MS` to guarantee renewal fires before expiry. | | `REDIS_EXEC_LOCK_ACQUIRE_TIMEOUT_MS` | No (default: 75000) | Max time to wait to acquire the exec lock before returning 503 (ms). Asserted at startup to be strictly greater than both `REDIS_EXEC_LOCK_LEASE_MS` and `REDIS_RWLOCK_READER_LEASE_MS` — a crashed holder's lock is only reaped when its lease expires, so a shorter window turns crashed-holder recovery into a 503. The default (lease + ~15s reap margin) also keeps the reply inside typical ingress timeouts (commonly 60-240s); the previous 300s sat above them, so the connection was severed and the 503 never reached the client. A caller that must queue behind a full-length 300s exec is expected to retry on the 503. | @@ -243,6 +242,35 @@ const TABLE = Object.assign(Object.create(null) as Record, { | `PG_DRIVER_FAULT_GUARD` | No (default: `true`) | Keeps the process alive when `postgres.js` throws out of its own socket-write path (#169): a backend reaped mid-transaction leaves the driver flushing a buffered write to a nulled socket from a bare `setImmediate`, a fatal uncaught exception that takes every other in-flight request on the replica with it. The guard recognises only that frame (`nextWrite` in `postgres/src/connection.js`, on a `TypeError`), logs `event:"driver_socket_fault"`, and fails the stuck DB awaits with `EDRIVERFAULT` → 503; every other uncaught exception and unhandled rejection keeps Node's default crash. Set `false` to restore crash-and-restart. | | `PG_DRIVER_FAULT_GRACE_MS` | No (default: 5000) | Grace window (ms) a DB await gets after a driver fault before it is failed with `EDRIVERFAULT`. The handler cannot attribute a fault to a connection, so the window spares healthy concurrent statements and bounds only the ones the driver dropped. | +### Redis eviction policy + +**The Redis carrying the data plane (`REDIS_DATA_URL`, defaulting to `REDIS_URL`) must run `maxmemory-policy allkeys-lru`** — `allkeys-lfu` is equally fine. No other policy is. + +Redis defaults to `noeviction`, under which an instance that reaches `maxmemory` refuses every write and **never recovers on its own**: blob cache entries carry a 24h TTL (`REDIS_BLOB_CACHE_TTL_MS`), so waiting it out is not a strategy and a human has to flush keys or raise the limit. The load harness measured 97.6% 5xx with no recovery, against a `CLIENT PAUSE` that recovered the moment the pause lifted (#188). With `allkeys-lru` the same pressure evicts cold blobs, which costs a Postgres read. + +Why those two and not the other evicting policies — on the default single-instance setup this policy governs the exec-lock leases, version counters and destroy tombstones as well as the cache: + +- `allkeys-lru` / `allkeys-lfu` protect exactly the keys that must not go. A lease renewed every `REDIS_EXEC_LOCK_RENEW_MS` (20s) and a version counter touched on every write are the most recently and most frequently used keys in the instance, so a cold blob is always the better candidate. +- `allkeys-random` samples uniformly, so a live lease is as likely to be reaped as the cold blob beside it. Not sufficient. +- `volatile-*` evicts **only** keys carrying a TTL. That is not about recency — the problem is that once the instance fills with keys that carry no TTL (the RW-lock reader ZSETs, anything a later change adds), it has no eviction candidate left and behaves exactly like `noeviction`: writes refused, no recovery without a human. `allkeys-*` always has a candidate. + +The check runs only when this client actually carries data-plane state (the blob cache or the path snapshot). With `REDIS_BLOB_CACHE_ENABLED=false` and no path snapshot, `REDIS_DATA_URL`'s fallback makes the "data" client the control instance, and a control-only Redis must NOT be switched to `allkeys-*` — that would make its leases and tombstones evictable. + +```bash +redis-cli CONFIG SET maxmemory-policy allkeys-lru # and persist it in redis.conf +``` + +Checked at boot via `CONFIG GET maxmemory-policy` (`src/redis/eviction-policy.ts`, wired in `server.ts`): + +| Outcome | Log line | +|---|---| +| `allkeys-lru` / `allkeys-lfu` | `event:"redis_eviction_policy"` | +| anything else | `event:"redis_eviction_policy_unsafe"`, `severity:"critical"` | +| `CONFIG GET` refused (`NOPERM`, unknown command) | `event:"redis_eviction_policy_unknown"`, `reason:"config_get_denied"` | +| `CONFIG GET` failed or unparseable | `event:"redis_eviction_policy_unknown"`, `reason:"config_get_failed"` | + +**It is a warning and never a startup failure**, and it is never awaited — managed providers routinely forbid `CONFIG GET`, and a Redis that answers slowly must not delay `listen`. + ## File Layout ``` @@ -262,6 +290,10 @@ src/ mysql/ ← MySQL DDL + stored procs azure-sql/ ← T-SQL DDL + RLS + stored procs integration/ ← DB integration tests (skippable) + redis/ ← Role-split clients, breaker, boot-time config checks + client.ts ← control/data ioredis clients + circuit-breaker.ts ← per-role breaker + eviction-policy.ts ← boot maxmemory-policy check (warn-only) api/ ← HTTP + MCP server server.ts ← Hono entry + migration runner auth.ts ← Bearer token middleware diff --git a/README.md b/README.md index 83151c27..d9bf5042 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,7 @@ server (or tests) at the stack: ```bash export DATABASE_URL=postgres://sqlfs_app:sqlfs_app@localhost:5432/sqlfs export REDIS_URL=redis://localhost:6379 +redis-cli CONFIG SET maxmemory-policy allkeys-lru # required — see below pnpm dev # or: pnpm test:integration ``` @@ -176,7 +177,7 @@ Key design choices: | `MAX_INGEST_FILES` | No | `10000` | Max number of entries (files + paths) in one `ingest-files` manifest. | | `MAX_INGEST_PATHS_CONCURRENCY` | No | `16` | Max concurrent host-file reads for the MCP `paths` ingest mode (bounds file descriptors / memory). | | `REDIS_URL` | No | — | Redis connection string. Required for multi-replica deployments. Without it, only the in-process mutex protects execution. Carries the control plane: locks, version counter, session state. | -| `REDIS_DATA_URL` | No | `REDIS_URL` | Connection string for the data plane (blob cache, path snapshot). A **separate connection** is opened either way, so multi-MiB cache writes cannot head-of-line block a lock command; point it at a different Redis only if you want physical separation too. | +| `REDIS_DATA_URL` | No | `REDIS_URL` | Connection string for the data plane (blob cache, path snapshot). A **separate connection** is opened either way, so multi-MiB cache writes cannot head-of-line block a lock command; point it at a different Redis only if you want physical separation too. This instance must run **`maxmemory-policy allkeys-lru`** (or `allkeys-lfu`) — see [Redis eviction policy](#redis-eviction-policy). | | `REDIS_EXEC_LOCK_LEASE_MS` | No | `60000` | Distributed exec lock TTL. Must be > `REDIS_EXEC_LOCK_RENEW_MS`. | | `REDIS_EXEC_LOCK_RENEW_MS` | No | `20000` | Lock heartbeat interval. Must be strictly less than lease. | | `REDIS_EXEC_LOCK_ACQUIRE_TIMEOUT_MS` | No | `75000` | Max wait to acquire exec lock before returning 503. Must be strictly greater than `REDIS_EXEC_LOCK_LEASE_MS` and `REDIS_RWLOCK_READER_LEASE_MS` (asserted at startup), so a crashed holder's lease can be reaped before the waiter gives up. | @@ -251,6 +252,46 @@ docker run -p 8080:8080 \ For multi-replica deployments, add `REDIS_URL`. All replicas share the same Postgres database and Redis instance; the exec lock ensures only one replica processes a given sandbox at a time. +### Redis eviction policy + +Configure the Redis behind `REDIS_DATA_URL` (which defaults to `REDIS_URL`) with +`allkeys-lru` (or `allkeys-lfu`): + +```bash +redis-cli CONFIG SET maxmemory-policy allkeys-lru # and persist it in redis.conf +``` + +Redis ships with `noeviction`, and under `noeviction` an instance that reaches +`maxmemory` starts refusing every write and **does not recover on its own**: blob +cache entries carry a 24h TTL (`REDIS_BLOB_CACHE_TTL_MS`), so waiting it out is not +a strategy, and someone has to flush keys or raise the limit by hand. The load +harness measured that as 97.6% 5xx with no recovery, against a `CLIENT PAUSE` that +recovered the moment the pause lifted. With `allkeys-lru` the same memory pressure +just evicts cold blobs, which costs a Postgres read. + +`allkeys-lru` and `allkeys-lfu` are the only two accepted, because on the default +single-instance setup this policy also governs the exec-lock leases, version +counters and destroy tombstones. Under LRU or LFU those are effectively immune — a +lease renewed every 20s and a counter touched on every write are the hottest keys +in the instance, so a cold blob is always the better candidate. `allkeys-random` +samples uniformly, so a live lease is as likely to be reaped as that cold blob. +`volatile-*` fails for a different reason: it evicts only keys that carry a TTL, so +once the instance fills with keys that do not, it has no candidate left and behaves +exactly like `noeviction`. + +The check runs only when this client actually carries the blob cache or the path +snapshot. With `REDIS_BLOB_CACHE_ENABLED=false` and no path snapshot the "data" +client is the control instance via the fallback, and a control-only Redis must +**not** be switched to `allkeys-*` — that would make its leases evictable. + +The server checks this at boot with `CONFIG GET maxmemory-policy` and logs +`event:"redis_eviction_policy_unsafe"` at `severity:"critical"` if the policy is +neither `allkeys-lru` nor `allkeys-lfu`. It is a warning, never a startup failure: managed Redis providers often +forbid `CONFIG GET`, and that case logs +`event:"redis_eviction_policy_unknown"` with `reason:"config_get_denied"` and boots +normally. If your provider hides the setting, confirm with them that the instance +evicts. + ## Development ```bash diff --git a/src/api/server.ts b/src/api/server.ts index 3cb44708..367b5e89 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -11,6 +11,7 @@ import type { ContentfulStatusCode } from "hono/utils/http-status"; import { getRedisCircuitBreaker } from "../redis/circuit-breaker.js"; import { closeRedisClient, getRedisClient } from "../redis/client.js"; import { parseNonNegativeInt, parsePositiveInt } from "../redis/config.js"; +import { startEvictionPolicyCheck } from "../redis/eviction-policy.js"; import { PostgresDialect } from "../sql-fs/dialects/postgres.js"; import { installDriverFaultGuard, raceDriverFault } from "../sql-fs/driver-fault.js"; import { translateSqlError } from "../sql-fs/errors.js"; @@ -352,6 +353,21 @@ if (isMain) { sessionManager.startReaper(); startMcpSessionSweeper(); + // #188: the Redis backing the blob cache must evict under memory pressure. + // Under `noeviction` a full instance refuses writes and never recovers, + // because blob entries carry a 24h TTL. Warn only, and never await: a + // managed Redis that refuses CONFIG GET must still boot. + // + // #188 M8: only when this client actually carries data-plane state. With + // the blob cache off and no path snapshot, `REDIS_DATA_URL`'s fallback + // makes the data client the CONTROL instance, and a control-only Redis + // holds nothing evictable worth trading: paging its operator to switch to + // allkeys-* would make the exec-lock leases, version counters and destroy + // tombstones evictable — the remediation would be the outage. + startEvictionPolicyCheck(redisDataClient, { + carriesDataPlane: Boolean(blobCacheEnabled) || Boolean(pathSnapshotEnabled), + }); + // F8: process-wide event-loop-lag monitor. Purely observational — surfaces // the GC-pause / sync-stall class that can silently void a Redis lease // (see event-loop-monitor.ts). The sampling timer is unref()'d internally. diff --git a/src/redis/eviction-policy.ts b/src/redis/eviction-policy.ts new file mode 100644 index 00000000..72c9c86f --- /dev/null +++ b/src/redis/eviction-policy.ts @@ -0,0 +1,194 @@ +/** + * Boot-time check on the eviction policy of the Redis backing the blob cache + * (#188). + * + * Blob cache entries carry a 24 h TTL (`REDIS_BLOB_CACHE_TTL_MS`). Under the + * Redis default `maxmemory-policy noeviction`, an instance that reaches + * `maxmemory` starts refusing every write with `OOM command not allowed` and + * does not recover on its own: waiting out a 24 h TTL is not an operational + * strategy, so a human has to flush keys or raise the limit. The harness + * measured that shape as 97.6% 5xx with no recovery, against a `CLIENT PAUSE` + * that recovered the moment the pause lifted. An `allkeys-lru` / `allkeys-lfu` policy turns + * the same event into eviction of cold blobs, which cost a Postgres read and + * nothing else — see `SAFE_POLICIES` for why those two and not the rest. + * + * This is a WARNING and never a startup failure. Managed Redis providers + * routinely refuse `CONFIG GET` (ElastiCache renames the command, Redis Cloud + * answers `NOPERM`), so a deployment that cannot answer the question must still + * boot. + */ + +import type { Redis } from "ioredis"; + +/** The subset of ioredis this check needs, so tests do not build a whole client. */ +export interface ConfigReader { + config(op: "GET", parameter: string): Promise; +} + +export type EvictionPolicyVerdict = "safe" | "unsafe" | "unreadable" | "denied"; + +export interface EvictionPolicyResult { + readonly verdict: EvictionPolicyVerdict; + readonly policy?: string; + readonly error?: string; +} + +/** + * True when the error says the provider will not answer `CONFIG GET` at all, as + * opposed to the connection being broken. Managed Redis hides or restricts the + * command in several different ways, and every one of them is a non-event: the + * operator cannot fix it, so the log line must not read like a misconfiguration. + */ +function isConfigDenied(message: string): boolean { + const lower = message.toLowerCase(); + // `OOM command not allowed when used memory > 'maxmemory'` contains "not + // allowed" but is the very failure this check exists to prevent, so it must + // not be filed as a benign permission answer. + if (lower.startsWith("oom")) return false; + return lower.startsWith("noperm") || lower.includes("unknown command") || lower.includes("not allowed"); +} + +/** ioredis answers `CONFIG GET` with a flat `[name, value]` array. */ +function policyFromReply(reply: unknown): string | undefined { + if (Array.isArray(reply)) { + const value = reply[1]; + return typeof value === "string" ? value : undefined; + } + // RESP3 clients hand back a map instead of a flat array. + if (reply !== null && typeof reply === "object") { + const value = (reply as Record)["maxmemory-policy"]; + return typeof value === "string" ? value : undefined; + } + return undefined; +} + +/** + * Reads `maxmemory-policy` and classifies it. Never throws and never rejects: + * the caller boots either way. + */ +export async function readEvictionPolicy(client: ConfigReader): Promise { + let reply: unknown; + try { + reply = await client.config("GET", "maxmemory-policy"); + } catch (err) { + const message = (err as Error)?.message ?? String(err); + return { verdict: isConfigDenied(message) ? "denied" : "unreadable", error: message }; + } + const policy = policyFromReply(reply); + if (policy === undefined) return { verdict: "unreadable" }; + return { verdict: SAFE_POLICIES.has(policy) ? "safe" : "unsafe", policy }; +} + +/** + * The only two policies that both always have something to evict and evict the + * right thing. + * + * By default the data role shares an instance with the control role + * (`REDIS_DATA_URL` falls back to `REDIS_URL`), so whatever policy is set + * governs the exec-lock leases, the version counters and the destroy + * tombstones as well as the blob cache. Under LRU or LFU those are effectively + * immune: a lease renewed every `REDIS_EXEC_LOCK_RENEW_MS` (20 s) and a version + * counter touched on every write are the most recently and most frequently used + * keys in the instance, so a cold blob is always the better candidate. + * + * `allkeys-random` is NOT accepted: it samples uniformly, so a live lease is + * exactly as likely to be reaped as the cold blob next to it. `volatile-*` is + * not accepted either, but for a different reason than recency — it evicts ONLY + * keys carrying a TTL, so an instance that fills with keys that do not carry one + * (the RW-lock reader ZSETs, anything a future change adds) has no eviction + * candidate left and degrades to exactly the `noeviction` failure this check + * exists to prevent: writes refused, no recovery without a human. `allkeys-*` + * always has a candidate. + */ +const SAFE_POLICIES: ReadonlySet = new Set(["allkeys-lru", "allkeys-lfu"]); + +/** Why this specific policy is refused — an operator log line has to be actionable. */ +function unsafeReason(policy: string | undefined): string { + if (policy === "allkeys-random") { + return "allkeys-random evicts uniformly at random, so a live exec-lock lease or version counter is as likely to be reaped as a cold blob; LRU/LFU never pick a key renewed every 20s."; + } + if (policy?.startsWith("volatile-") === true) { + return "volatile-* evicts only keys that carry a TTL, so once the instance fills with keys that do not, it has no candidate left and behaves exactly like noeviction: writes refused, no recovery without a human."; + } + return "under noeviction a Redis at maxmemory refuses every write and does not recover, because blob entries carry a 24h TTL."; +} + +/** + * Runs {@link readEvictionPolicy} and logs the verdict. Returns the result so + * tests and callers can assert on it without parsing stderr. + */ +export async function checkEvictionPolicy( + client: ConfigReader, + sink?: (line: string) => void, +): Promise { + const result = await readEvictionPolicy(client); + // An unsafe policy is an operator action item, so it goes to stderr at + // critical severity; the rest are informational. + const log = sink ?? (result.verdict === "unsafe" ? console.error : console.warn); + switch (result.verdict) { + case "safe": + log(JSON.stringify({ event: "redis_eviction_policy", policy: result.policy })); + break; + case "unsafe": + log( + JSON.stringify({ + event: "redis_eviction_policy_unsafe", + severity: "critical", + policy: result.policy, + message: `Redis maxmemory-policy is "${result.policy}". The blob cache needs allkeys-lru (or allkeys-lfu): ${unsafeReason(result.policy)} Run CONFIG SET maxmemory-policy allkeys-lru (and persist it in redis.conf).`, + }), + ); + break; + case "denied": + log( + JSON.stringify({ + event: "redis_eviction_policy_unknown", + reason: "config_get_denied", + error: result.error, + message: + "Could not read maxmemory-policy: this Redis does not allow CONFIG GET. " + + "Confirm with your provider that the instance evicts (allkeys-lru or equivalent).", + }), + ); + break; + case "unreadable": + log( + JSON.stringify({ + event: "redis_eviction_policy_unknown", + reason: "config_get_failed", + error: result.error, + }), + ); + break; + } + return result; +} + +export interface EvictionPolicyCheckOptions { + /** + * Whether this client actually carries data-plane state (blob cache or path + * snapshot). Defaults to `true`; pass `false` to skip the check entirely. + * + * #188 M8: `REDIS_DATA_URL` falls back to `REDIS_URL`, so with the blob cache + * disabled and no path snapshot the "data" client IS the control instance. A + * control-only Redis holds leases, version counters and destroy tombstones and + * no cache at all — nothing there is worth evicting, and the remediation this + * check pages for (switch to `allkeys-*`) would make all of it evictable. So a + * correctly-configured control-only deployment must not be paged at all. + */ + readonly carriesDataPlane?: boolean; +} + +/** + * Fire-and-forget boot hook. Deliberately not awaited by the caller: a Redis + * that answers slowly must not hold up `listen`, and a rejection here must not + * become an unhandled rejection. + */ +export function startEvictionPolicyCheck(client: Redis | undefined, opts: EvictionPolicyCheckOptions = {}): void { + if (client === undefined) return; + if (opts.carriesDataPlane === false) return; + void checkEvictionPolicy(client).catch(() => { + // checkEvictionPolicy already swallows; this is belt-and-braces so a + // future change there can never crash boot. + }); +} diff --git a/src/redis/tests/eviction-policy.test.ts b/src/redis/tests/eviction-policy.test.ts new file mode 100644 index 00000000..0e36020b --- /dev/null +++ b/src/redis/tests/eviction-policy.test.ts @@ -0,0 +1,223 @@ +/** + * US-188 unit tests: the boot-time `maxmemory-policy` check. + * + * The check exists because `noeviction` turns a full Redis into a permanent + * outage rather than a transient one (blob entries carry a 24 h TTL, so waiting + * it out is not a strategy). It must warn loudly, never fail startup, and tell a + * provider that forbids `CONFIG GET` apart from a provider that answered badly. + */ + +import { describe, expect, it, vi } from "vitest"; +import { + type ConfigReader, + checkEvictionPolicy, + readEvictionPolicy, + startEvictionPolicyCheck, +} from "../eviction-policy.js"; + +function reader(reply: unknown): ConfigReader { + return { config: vi.fn(async () => reply) }; +} + +function failing(message: string): ConfigReader { + return { + config: vi.fn(async () => { + throw new Error(message); + }), + }; +} + +/** Captures the lines the check emits so assertions read the JSON, not stderr. */ +function sink(): { lines: string[]; log: (line: string) => void } { + const lines: string[] = []; + return { lines, log: (line) => lines.push(line) }; +} + +describe("readEvictionPolicy (US-188)", () => { + it("accepts the two recency/frequency policies", async () => { + for (const policy of ["allkeys-lru", "allkeys-lfu"]) { + expect(await readEvictionPolicy(reader(["maxmemory-policy", policy]))).toEqual({ verdict: "safe", policy }); + } + }); + + // #188 M9: `allkeys-random` used to be blessed by a `startsWith("allkeys-")` + // test. It evicts uniformly, so a lease renewed every 20s is as likely to be + // reaped as the cold blob beside it — which is the very thing the rejection of + // volatile-* was justified by. LRU/LFU are the policies for which that + // argument actually holds. + it("rejects allkeys-random — uniform sampling can reap a live lease", async () => { + expect(await readEvictionPolicy(reader(["maxmemory-policy", "allkeys-random"]))).toEqual({ + verdict: "unsafe", + policy: "allkeys-random", + }); + }); + + it("rejects noeviction and every volatile policy", async () => { + // volatile-* evicts only keys carrying a TTL, so an instance that fills + // with keys that do not carry one has no candidate left and degrades to + // noeviction: writes refused, no recovery without a human. + for (const policy of ["noeviction", "volatile-lru", "volatile-lfu", "volatile-random", "volatile-ttl"]) { + expect(await readEvictionPolicy(reader(["maxmemory-policy", policy]))).toEqual({ verdict: "unsafe", policy }); + } + }); + + it("reads a RESP3 map reply as well as a flat array", async () => { + expect(await readEvictionPolicy(reader({ "maxmemory-policy": "allkeys-lru" }))).toEqual({ + verdict: "safe", + policy: "allkeys-lru", + }); + }); + + it("reports a reply it cannot parse as unreadable rather than guessing", async () => { + for (const reply of [[], ["maxmemory-policy"], null, "allkeys-lru", 7]) { + expect(await readEvictionPolicy(reader(reply))).toEqual({ verdict: "unreadable" }); + } + }); + + it("classifies the ways a managed provider refuses CONFIG GET as denied", async () => { + for (const message of [ + "NOPERM this user has no permissions to run the 'config|get' command", + "ERR unknown command 'config', with args beginning with: 'GET'", + "ERR CONFIG GET is not allowed on this instance", + ]) { + expect(await readEvictionPolicy(failing(message))).toEqual({ verdict: "denied", error: message }); + } + }); + + it("does not file a transport failure or an OOM refusal as denied", async () => { + // Negative guard: "denied" is the quiet verdict, so anything that is + // actually wrong with the instance must not land there. `OOM command not + // allowed…` contains "not allowed" and is the exact failure this check + // exists to prevent. + for (const message of [ + "OOM command not allowed when used memory > 'maxmemory'.", + "Command timed out", + "ECONNRESET", + "Connection is closed.", + ]) { + expect(await readEvictionPolicy(failing(message))).toEqual({ verdict: "unreadable", error: message }); + } + }); +}); + +describe("checkEvictionPolicy logging (US-188)", () => { + it("logs a critical warning naming the policy and the remedy when it is unsafe", async () => { + const out = sink(); + await checkEvictionPolicy(reader(["maxmemory-policy", "noeviction"]), out.log); + + expect(out.lines).toHaveLength(1); + const line = JSON.parse(out.lines[0] as string) as Record; + expect(line.event).toBe("redis_eviction_policy_unsafe"); + expect(line.severity).toBe("critical"); + expect(line.policy).toBe("noeviction"); + expect(line.message).toContain("allkeys-lru"); + }); + + // The remedy line is what an operator acts on, so each family has to be told + // WHY its policy was refused — "not allkeys-*" is not a reason for a policy + // that is allkeys-*. + it("explains the refusal in terms of the policy it actually found", async () => { + const cases: Array<[string, string]> = [ + ["allkeys-random", "uniformly at random"], + ["volatile-lru", "only keys that carry a TTL"], + ["noeviction", "refuses every write"], + ]; + for (const [policy, reason] of cases) { + const out = sink(); + await checkEvictionPolicy(reader(["maxmemory-policy", policy]), out.log); + const line = JSON.parse(out.lines[0] as string) as Record; + expect(line.event).toBe("redis_eviction_policy_unsafe"); + expect(line.message).toContain(reason); + expect(line.message).toContain("allkeys-lru"); + } + }); + + it("does not emit the unsafe event when the policy is safe", async () => { + // Negative guard: a warning on every healthy boot would train operators to + // ignore the one that matters. + const out = sink(); + await checkEvictionPolicy(reader(["maxmemory-policy", "allkeys-lru"]), out.log); + + expect(out.lines).toHaveLength(1); + expect(JSON.parse(out.lines[0] as string)).toEqual({ event: "redis_eviction_policy", policy: "allkeys-lru" }); + }); + + it("distinguishes a refused CONFIG GET from a failed one in the log", async () => { + const denied = sink(); + await checkEvictionPolicy(failing("NOPERM this user has no permissions"), denied.log); + const failed = sink(); + await checkEvictionPolicy(failing("Command timed out"), failed.log); + + expect(JSON.parse(denied.lines[0] as string).reason).toBe("config_get_denied"); + expect(JSON.parse(failed.lines[0] as string).reason).toBe("config_get_failed"); + }); + + it("never rejects, whatever CONFIG GET does", async () => { + // The whole point is that startup survives this check. + const out = sink(); + await expect(checkEvictionPolicy(failing("boom"), out.log)).resolves.toMatchObject({ verdict: "unreadable" }); + const thrower: ConfigReader = { + config: () => { + throw new Error("synchronous throw"); + }, + }; + await expect(checkEvictionPolicy(thrower, out.log)).resolves.toMatchObject({ verdict: "unreadable" }); + }); +}); + +describe("startEvictionPolicyCheck (US-188)", () => { + it("is a no-op when no Redis is configured", () => { + expect(() => startEvictionPolicyCheck(undefined)).not.toThrow(); + }); + + // #188 M8: `REDIS_DATA_URL` falls back to `REDIS_URL`, so with the blob cache + // off and no path snapshot the "data" client IS the control instance. Paging + // its operator to switch to allkeys-* would make the exec-lock leases and + // tombstones evictable — the remediation would be the outage. + it("does not touch Redis when the client carries no data-plane state", () => { + const config = vi.fn(async () => ["maxmemory-policy", "noeviction"]); + const client = { config } as unknown as Parameters[0]; + const err = vi.spyOn(console, "error").mockImplementation(() => {}); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + startEvictionPolicyCheck(client, { carriesDataPlane: false }); + + expect(config).not.toHaveBeenCalled(); + expect(err).not.toHaveBeenCalled(); + expect(warn).not.toHaveBeenCalled(); + err.mockRestore(); + warn.mockRestore(); + }); + + it("still checks when the client carries data-plane state", async () => { + const config = vi.fn(async () => ["maxmemory-policy", "noeviction"]); + const client = { config } as unknown as Parameters[0]; + const err = vi.spyOn(console, "error").mockImplementation(() => {}); + + startEvictionPolicyCheck(client, { carriesDataPlane: true }); + await new Promise((r) => setImmediate(r)); + + expect(config).toHaveBeenCalledTimes(1); + expect(JSON.parse(err.mock.calls[0]?.[0] as string).event).toBe("redis_eviction_policy_unsafe"); + err.mockRestore(); + }); + + it("returns synchronously without awaiting the Redis round trip", async () => { + let resolveConfig: (v: unknown) => void = () => {}; + const pending = new Promise((r) => { + resolveConfig = r; + }); + const client = { config: vi.fn(() => pending) } as unknown as Parameters[0]; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + startEvictionPolicyCheck(client); + // Boot continues while CONFIG GET is still in flight. + expect(warn).not.toHaveBeenCalled(); + + resolveConfig(["maxmemory-policy", "allkeys-lru"]); + await pending; + await new Promise((r) => setImmediate(r)); + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); +});