Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 8bb36ad. Bugbot is set up for automated code reviews on this repo. Configure here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward enforcement of Redis as a required dependency.
Extended reasoning...
Overview
Single-file change to run.go that removes the ErrRedisDisabled carve-out and all redisClient \!= nil guards, making Redis unconditionally required at startup. The companion commit (21aa127) already establishes this as an intentional infrastructure decision.
Security risks
None. The change removes optional code paths rather than introducing new ones; it tightens startup requirements rather than relaxing any security boundary.
Level of scrutiny
Low. This is a small, mechanical removal of fallback/optional logic with clear and explicit intent. No new logic is introduced — only guards are deleted. The fatal-on-error path already existed for other error types.
Other factors
One nit was flagged (inline comment): when ErrRedisDisabled is returned, the log message 'Could not connect to Redis' may mislead operators into debugging connectivity rather than checking for a missing env var. This is a diagnostic quality issue only — the behavior (fatal exit) is correct and intended. The structured zap.Error(err) field will include 'redis is disabled' for operators who look, mitigating the impact.
| if err != nil { | ||
| logger.L().Fatal(ctx, "Could not connect to Redis", zap.Error(err)) | ||
| } else if err == nil { | ||
| closers = append(closers, closer{"redis client", func(context.Context) error { | ||
| return sharedFactories.CloseCleanly(redisClient) | ||
| }}) | ||
| } |
There was a problem hiding this comment.
🟡 When neither RedisURL nor RedisClusterURL is set, NewRedisClient returns ErrRedisDisabled (no connection is ever attempted), but the new code logs 'Could not connect to Redis' — implying a network or auth failure. While zap.Error(err) will include 'redis is disabled' in the structured log, operators may waste time investigating connectivity instead of checking for a missing env var. Consider detecting ErrRedisDisabled explicitly and logging 'Redis is required but not configured'.
Extended reasoning...
What the bug is: When neither RedisURL nor RedisClusterURL is configured, sharedFactories.NewRedisClient returns ErrRedisDisabled (message: "redis is disabled") with a nil client — no TCP connection is ever attempted. After this PR removes the ErrRedisDisabled exclusion, the generic if err \!= nil branch fires and logs a fatal with the primary message "Could not connect to Redis".
The specific code path: run.go:386-388 — the new if err \!= nil block no longer carves out ErrRedisDisabled, so a missing-config error and a genuine connection failure (wrong host, auth failure, TLS mismatch) both surface identically as "Could not connect to Redis".
Why existing code doesn't prevent it: The zap.Error(err) field will emit the underlying error text "redis is disabled", which partially mitigates the confusion. However, structured-log fields are often less prominent than the message string in dashboards, alerting rules, or plain-text log tailing, so the misleading primary message is what operators see first.
Impact: An operator whose deploy fails because they forgot to set REDIS_URL or REDIS_CLUSTER_URL will see "Could not connect to Redis" and naturally start debugging network connectivity, firewall rules, TLS certificates, or Redis auth — none of which are the problem.
Addressing the refutation: The refutation is correct that the behavior (fatal exit when Redis is unconfigured) is intentional and correct — this PR explicitly makes Redis required. The bug is only in the diagnostic quality of the log message. The fix does not change semantics; it simply distinguishes the two failure modes.
Proof (concrete example): Deploy orchestrator without setting any Redis env vars → NewRedisClient short-circuits at the URL check, returns (nil, ErrRedisDisabled) (redis.go:100) → err \!= nil is true → fatal: "Could not connect to Redis" {error: "redis is disabled"}. An operator reading the log message alone diagnoses a connection failure, not a misconfiguration.
Fix: if errors.Is(err, sharedFactories.ErrRedisDisabled) { logger.L().Fatal(ctx, "Redis is required but not configured", zap.Error(err)) } else if err \!= nil { logger.L().Fatal(ctx, "Could not connect to Redis", zap.Error(err)) }
There was a problem hiding this comment.
1/5 (basically what Claude said ^^) Right now the error message a developer would see is "redis is disabled" wrapped inside "Could not connect to Redis", which is a bit misleading - they didn't fail to connect, they failed to configure. A dedicated config validation at startup ("REDIS_URL or REDIS_CLUSTER_URL must be set") would be clearer.
Are local runs affected by this change?
| PoolSize: config.RedisPoolSize, | ||
| }) | ||
| if err != nil && !errors.Is(err, sharedFactories.ErrRedisDisabled) { | ||
| if err != nil { |
There was a problem hiding this comment.
1/5 client-proxy/main.go still has a similar check, should it be eliminated also?
2 participants
No description provided.