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 6 commits into
Conversation
|
|
0aabfff to
418e509
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
|
@danny-avila all issues should be fixed Added : Merged from upstream to fix conflicts |
0deb596 to
045d5a2
Compare
… 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.
- Hash-tag per-execution Redis keys ({execution_id}) in replay-state and
tool-call-server so multi-key Lua scripts, MULTI/EXEC and multi-key DELs
stay on one Cluster slot (avoids CROSSSLOT errors)
- Add shared hashTag/stripHashTag/scanKeys helpers in redis-connection
- Replace blocking KEYS with cluster-aware SCAN in tool-call-server cleanup
- Fix wait-for-redis probe to target a single startup node in cluster mode
via new codeapi.redis.probeHost/probePort helpers
- Gate REDIS_TLS/CA env and volumes behind external Redis (not bundled subchart)
- Document redis.enabled=false requirement for cluster mode in README
045d5a2 to
7477efe
Compare
|
Rebased yet again :) |
|
@codex review the latest head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7477efe4e7
ℹ️ 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".
| export function hashTag(id: string): string { | ||
| return `{${id}}`; |
There was a problem hiding this comment.
On any ordinary standalone deployment upgraded while a PTC execution is active, this unconditional transformation changes every key from forms such as exec_state:<id> and tool_call:session:<id> to braced variants. The new processes therefore cannot find state written by the previous version, and mixed-version pods in a rolling deployment cannot interoperate, causing continuations and blocking tool calls to fail for up to the 10-minute TTL even though cluster mode is disabled. Return the raw ID outside cluster mode or dual-read/migrate legacy keys during rollout.
Useful? React with 👍 / 👎.
| if (isClusterMode()) { | ||
| const nodes = parseRedisNodes(); | ||
| return new Cluster(nodes, { |
There was a problem hiding this comment.
When USE_REDIS_CLUSTER=true and runtime sessions use affinity or strict mode, this Cluster client is also consumed by runtime-session/registry.ts, whose record-write and removal scripts pass rtsx:sess:<id> and rtsx:lock:<id> as two keys without a common hash tag. Although the replay keys are now tagged, these unchanged registry scripts still fail with CROSSSLOT, preventing stateful executions from persisting or removing their session records; tag all registry keys for a runtime-session ID consistently.
Useful? React with 👍 / 👎.
| redis: | ||
| enabled: false # required — otherwise the bundled subchart is used instead | ||
| cluster: | ||
| enabled: true | ||
| nodes: 'node-1:6379,node-2:6379,node-3:6379' |
There was a problem hiding this comment.
Using this new example verbatim sets redis.enabled=false without defining redis.external, which is also absent from the chart defaults. The chart still evaluates .Values.redis.external.port in codeapi.redis.port and .Values.redis.external.password in templates/secrets.yaml, so Helm aborts with a nil-pointer template error before a cluster deployment can be installed. Add cluster-specific port/password handling or provide a default redis.external map in the example/default values.
Useful? React with 👍 / 👎.
| const batchKeys = keys.slice(offset, offset + CLEANUP_BATCH_SIZE); | ||
| const values = await redis.mget(...batchKeys); |
There was a problem hiding this comment.
When Redis Cluster contains state for two or more executions, scanKeys returns keys tagged with different execution IDs and this batch passes them together to MGET. Redis Cluster requires every key in a multi-key command to share a slot, so the call normally raises CROSSSLOT; the outer catch then returns zero and prevents the entire stale-execution cleanup pass from running. Fetch these keys individually, use a cluster-aware pipeline, or group them by hash slot.
Useful? React with 👍 / 👎.
| const ca = readCACert(); | ||
| if (ca) return { ca }; | ||
| if (process.env.REDIS_TLS === 'true') return { rejectUnauthorized: false }; |
There was a problem hiding this comment.
When an operator sets both REDIS_CA and REDIS_TLS=true but the CA path is missing or unreadable, readCACert() returns null and this branch silently falls back to rejectUnauthorized: false. A typo or permissions problem therefore downgrades a deployment that explicitly requested CA validation to unauthenticated TLS while merely logging a warning; distinguish an unset CA from a failed configured CA and refuse to create the connection in the latter case.
Useful? React with 👍 / 👎.
| const trimmed = entry.trim(); | ||
| const colonIdx = trimmed.lastIndexOf(':'); | ||
| if (colonIdx > 0) { | ||
| const potentialPort = Number(trimmed.slice(colonIdx + 1)); | ||
| if (Number.isInteger(potentialPort) && potentialPort > 0) { | ||
| return { |
There was a problem hiding this comment.
When REDIS_HOST contains a literal IPv6 startup node, this last-colon heuristic misparses a plain address such as 2001:db8::1 as host 2001:db8: on port 1; the standard bracketed form [2001:db8::1]:6379 instead leaves the brackets in the host, which Node DNS cannot resolve. Redis therefore cannot connect in IPv6-only environments. Parse bracketed IPv6 explicitly and treat an unbracketed value containing multiple colons as a host without an embedded port; the Helm probe helpers need the corresponding handling.
Useful? React with 👍 / 👎.
Resolve conflicts in service/src/config.ts, queue.ts, workers.ts: - Keep main's execution-profile logic (EXECUTION_PROFILE, queueNames, validateQueuedExecutionProfile, profile trace attributes) - Keep feature's Redis cluster changes (bullmqPrefix on Queue/QueueEvents/ Worker, USE_REDIS_CLUSTER env) - Preserve canonical 4-space prettier formatting Pre-existing typecheck errors (RedisClient vs Redis in registry/replay-state, disconnectTimeout in redis-connection) are unchanged from the feature branch tip and tracked by open PR review comments.
Resolve the PR-base conflicts while preserving both upstream execution routing and Redis Cluster support, including cluster-aware queue prefixes and replay state keys.
3 participants
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