Repository navigation
Conversation
go-redis replaces only a pool_size of 0 with its default and panics building a client from a negative one. ParseRedisURL accepted it, so the admin connection check answered 500 with a handler panic, the save was accepted, and the next start panicked before an admin could fix it. Since Sentinel URLs became supported, the same holds for them. Refuse a negative pool_size in both forms with an error that names the option, so the save answers 422 and the connection check reports a failed check. Other negative pool options are left alone: go-redis ignores them, so they start today.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
Problem
Closes #1862
Related issue: #1861
Validation tasks: none
A
redis.urlwith a negativepool_size(for exampleredis://redis:6379?pool_size=-1) passes validation, and go-redis panics when a client is built from it. The admin connection check answers 500 with a handler panic, the save is accepted, and the next start panics before anyone can fix the value in the admin UI. #1862 has the full reproduction. This change refuses a negativepool_sizewhen the URL is parsed, so the save answers 422 naming the option and the connection check reports a failed check, as they already do forpool_size=many.go-redis replaces a
pool_sizeof 0 with its default, but uses any other value as the size of its pool's semaphore channel, so a negative one panics inNewClientwithmakechan: size out of range. Since Sentinel URLs became supported (#1859), the same happens for them: the master client gets the same pool size.Approach
ParseRedisURLreturnsredis: pool_size must be 0 or more; leave it out for the defaultfor a negativepool_size, for both single-server and Sentinel URLs.NormalizeRedisURL, the connection check and startup all go through it, so the save refuses it and nothing builds a client from it.min_idle_conns,max_idle_conns,max_active_conns,max_concurrent_dials) are left alone. go-redis ignores or clamps them, and refusing them would stop a server that starts today, because startup exits when its saved URL can't be used (A savedredis.urlthat Silo cannot connect with stops the server at the next start #1861).A server that already has
pool_size=-1saved still can't start, but with a clear error instead of a panic. Recovering from a saved URL that doesn't work is #1861.Validation
internal/configrefusepool_size=-1in single-server and Sentinel URLs, check the error names the option, build a go-redis client from every pool sizeParseRedisURLaccepts (0 and 20 still work), and checkNormalizeRedisURLrefuses it. On ca186fe they fail: both URLs parse, building a client panics withmakechan: size out of range, and the save accepts it.go test ./internal/config/passes.go vetandmake lint-changed: clean.Benchmarks
Not applicable. This only adds a check when the URL is parsed.
Evidence
Evidence: https://evidence.siloserver.org/r/silo-server/pr-2079/
Before: the reproduction in #1862 and the failing tests. After: the save refuses the URL with an error naming
pool_size.Risks
None identified. A negative
pool_sizecould never build a client, so no working setup is refused.Checklist
AI Disclosure
AI-assisted with Claude Opus. I directed the task and designed the work.