feat: add Redis Cluster mode support (GCP Memorystore, AWS ElastiCache)#17
feat: add Redis Cluster mode support (GCP Memorystore, AWS ElastiCache)#17pedrojreis wants to merge 3 commits into
Conversation
… handling * Refactor job processing in workers.ts for improved readability and maintainability. * Introduce Redis connection management in redis-connection.ts. * Add tests for Redis connection utilities in redis-connection.test.ts. * Implement TLS options handling for secure Redis connections. * Enhance error handling and logging throughout the job processing flow.
* Updated the project dependency to version 2.3.1. * Ensured compatibility with existing codebase. * Ran tests to verify functionality post-upgrade.
0aabfff to
418e509
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418e509eb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (isClusterMode()) { | ||
| const nodes = parseRedisNodes(); | ||
| return new Cluster(nodes, { |
There was a problem hiding this comment.
Hash-tag replay keys before enabling Redis Cluster
When USE_REDIS_CLUSTER is true this returns an ioredis Cluster, but replay state still performs multi-key Lua/transactions over untagged keys such as exec_state:<id>, exec_result:<id>, and tool_history:<id> in setExecutionResultScript and commitToolHistoryAndState. In Redis Cluster those key names hash to different slots, so PTC completion/continuation paths fail with CROSSSLOT once cluster mode is enabled; hash-tag the per-execution keys (or avoid multi-key ops) before constructing Cluster clients here.
Useful? React with 👍 / 👎.
| {{- if .Values.redis.enabled }} | ||
| {{- printf "%s-redis-master" .Release.Name }} | ||
| {{- else if and .Values.redis.cluster.enabled .Values.redis.cluster.nodes }} | ||
| {{- .Values.redis.cluster.nodes }} |
There was a problem hiding this comment.
Probe one Redis startup node in cluster mode
With the documented Helm setting redis.cluster.nodes="n1:6379,n2:6379,n3:6379", this helper renders the entire comma-separated list into the worker wait-for-redis initContainer command (nc -z {{ include "codeapi.redis.host" . }} {{ include "codeapi.redis.port" . }}). nc -h documents the syntax as [destination] [port], so the init container tries to resolve a literal destination like n1:6379,n2:6379,n3:6379 and never lets worker pods start; render/probe a single startup node or skip this probe for cluster mode.
Useful? React with 👍 / 👎.
| **Redis Cluster mode (GCP Memorystore cluster, AWS ElastiCache cluster).** Set | ||
| `redis.cluster.enabled=true` and provide the comma-separated startup nodes in | ||
| `redis.cluster.nodes`. The chart will set `USE_REDIS_CLUSTER=true` and populate | ||
| `REDIS_HOST` with the node list on every component. For TLS with a CA certificate |
There was a problem hiding this comment.
Document disabling the bundled Redis for clusters
Following this new cluster-mode README snippet with the chart defaults leaves redis.enabled as true; codeapi.redis.host then gives precedence to the bundled standalone Redis service while USE_REDIS_CLUSTER=true, so the pods instantiate an ioredis Cluster client against <release>-redis-master and ignore the provided startup nodes. Include redis.enabled=false in the example or make the helper/validation reject this conflicting configuration.
Useful? React with 👍 / 👎.
| ...(useAltDnsLookup | ||
| ? { dnsLookup: (address: string, callback: (err: Error | null, addr: string) => void): void => callback(null, address) } | ||
| : {}) | ||
| const redis = createRedisConnection({ |
There was a problem hiding this comment.
Hash-tag tool-call session keys before Cluster cleanup
When this connection becomes an ioredis Cluster, the blocking-mode delete path still gathers tool_call:* keys and calls redis.del(...keys) for a session. A normal session can have multiple untagged keys (session, call, result/error) that hash to different slots, so cleanup fails with CROSSSLOT or misses keys under Redis Cluster; tag the tool-call keys by execution id or delete them one key/node at a time.
Useful? React with 👍 / 👎.
| {{- if .Values.redis.tls.enabled }} | ||
| - name: REDIS_TLS | ||
| value: "true" |
There was a problem hiding this comment.
Gate Redis TLS env on external Redis
The new values block says Redis TLS is for external managed Redis and is ignored when the bundled Redis is enabled, but this helper emits REDIS_TLS=true whenever redis.tls.enabled is set. If an operator enables that TLS block while leaving the chart default redis.enabled=true, every component tries to speak TLS to the plain in-cluster Bitnami Redis service and fails to connect; either suppress these env vars when redis.enabled is true or configure TLS on the bundled subchart too.
Useful? React with 👍 / 👎.
Overview
Adds opt-in Redis Cluster support to every service component. Standalone
Redis remains the default — existing deployments require zero configuration
changes and behave exactly as before.
Validated in production against Google Cloud Memorystore in cluster mode with
TLS and CA-certificate verification.
Motivation
The service previously constructed Redis connections with inline
new IORedis({ ... })calls in four separate modules, each hardcoded tostandalone mode. Connecting to a clustered Redis (GCP Memorystore cluster, AWS
ElastiCache cluster) was impossible: the client would only ever reach a single
shard and fail with
MOVED/CROSSSLOTerrors under load.This PR centralizes connection creation behind a single factory and teaches
every component to speak the Redis Cluster protocol when asked.
What's new
🔌 Cluster mode (opt-in, auto-detected)
Enable it either explicitly or implicitly:
🔐 TLS with CA-certificate validation
When
REDIS_CAis set it takes precedence and enables validated TLS.REDIS_TLS=trueon its own keeps the previousrejectUnauthorized: falsebehaviour for backward compatibility.
🧩 BullMQ cluster-safety
Queue, Worker and QueueEvents receive a
{codeapi}hash-tag prefix in clustermode so all BullMQ keys map to a single hash slot (a hard requirement for BullMQ
on Redis Cluster). Standalone deployments keep their existing key layout — no
migration needed.
New environment variables
USE_REDIS_CLUSTERfalseREDIS_HOSTcontains a comma.REDIS_CAREDIS_TLS.Existing variables are unchanged and fully backward-compatible:
REDIS_HOST,REDIS_PORT,REDIS_PASSWORD,REDIS_TLS,REDIS_USE_ALTERNATIVE_DNS_LOOKUP,REDIS_KEEP_ALIVE_MS.Implementation
service/src/redis-connection.ts(new — single source of truth)createRedisConnection(overrides)Redis | Clusterbased on env; each caller passes its own retry / readyCheck overridesisClusterMode()USE_REDIS_CLUSTER=trueor comma inREDIS_HOSTparseRedisNodes()REDIS_HOSTinto[{ host, port }]startup nodesbuildTlsOptions()REDIS_CA→{ ca }(validated); elseREDIS_TLS=true→{ rejectUnauthorized: false }; else no TLSbullmqPrefix()'{codeapi}'in cluster mode,undefinedotherwiseRefactored clients
All four inline
new IORedis({ ... })blocks now callcreateRedisConnection():queue.ts— shared BullMQ connection +prefix: bullmqPrefix()onQueue/QueueEventsworkers.ts—prefix: bullmqPrefix()on bothWorkerinstancesegress-ledger.ts— mutation-connection pool made cluster-safe (Clusterhas no.duplicate(), so a freshcreateRedisConnection()is used instead)tool-call-server.ts,file-server.ts— session-state clientsservice/src/service/replay-state.tsscanKeys()is now cluster-aware.ioredis.Clusterhas no top-levelscanStream, so in cluster mode the helper fans out across every master nodevia
cluster.nodes('master')and streamsSCANon each. Masters own disjointhash-slot ranges, so results never overlap. This fixes the runtime crash:
service/src/config.tsAdds the
USE_REDIS_CLUSTERflag to the parsed env.Helm chart (
helm/codeapi/)New
values.yamlsurface:New
_helpers.tpltemplates —codeapi.redis.clusterEnabled,codeapi.redis.tlsEnv,codeapi.redis.caVolume,codeapi.redis.caVolumeMount— are wired into all five component Deployments, including mounting the CA cert
from a Secret into each pod.
service/.env.exampleDocuments every new variable with inline guidance.
Tests
New
service/src/redis-connection.test.ts— 18 unit tests, no live Redis required:parseRedisNodesisClusterModebuildTlsOptionsREDIS_TLSonly,REDIS_CAfile read, CA precedence overREDIS_TLS, missing CA filebullmqPrefixBackward compatibility
REDIS_TLS=truewithoutREDIS_CAkeeps the priorrejectUnauthorized: falsebehaviour.How to verify