US-188: require allkeys-lru on the blob-cache Redis and warn at boot

.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.

CLAUDE.md

@@ -217,8 +217,7 @@ const TABLE = Object.assign(Object.create(null) as Record<string, string>, {
 | `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=<token>` 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<string, string>, {
 | `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

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