Repository navigation
feat(jawn): add official Valkey backend support with CI and docs - #1
rileydes-improving wants to merge 1 commit into
Conversation
daric93
left a comment
There was a problem hiding this comment.
Code Review: feat(jawn): add official Valkey backend support with CI and docs
Thanks for this — it's a clean, low-risk contribution, and adding CI + docs to certify existing compatibility is genuinely useful for operators evaluating the stack. My main concern is that the docs and CI over-promise relative to what the code and tests actually deliver.
What works well
- Zero production code change is the right call — Redis operators are completely unaffected, and the compatibility claim stands on its own.
- ~330 lines of well-structured tests with good edge-case coverage on the parts they do test: TTL expiry, LRU-style eviction, AES-GCM round-trip, random-IV non-determinism, corrupted-ciphertext rejection, and the "errors aren't cached" path in
getAndStoreInCache. describeIfRedisgating cleanly separates pure-unit sections from those needing a live instance.
Issues requiring changes
Bugs / Docs (blocking):
- Docs promise "just set
REDIS_HOST, no other config," but the production client hardcodestls: {}with no toggle — the plain-Valkey docker-compose example in the same doc would fail the TLS handshake. The test even mocks around this exact fact. - "What's Verified — tested against Valkey in CI" lists 5 subsystems; the test only exercises 2. The two rate limiters and the Usage Limit Cache are never imported by any test.
Test coverage:
3. The commands most worth proving (Lua SCRIPT LOAD/EVALSHA/PEXPIRE, the EX sliding window) are exactly the untested ones — a GET/SET round-trip has been Valkey-safe since the fork.
Design / Test fidelity:
5. CI certifies a hand-rolled ioredis client (conditional TLS, custom options), not the production singleton — so "drop-in compatible" is proven for a client prod never uses.
6. CI pins Valkey to the moving latest tag, disagreeing with the PR's valkey:8-alpine and the docs' "Valkey 8 or later."
Nits:
4. "serves from memory ... no extra Redis call" never asserts the second get skips Redis.
7. "Valkey 8 or later" is stricter than any listed command requires (all exist in 7.2).
Recommendations
- Fix #1 and Helicone#2 before merge — for a docs+CI PR, doc accuracy is the deliverable, and both describe behavior that doesn't hold (a connection path that fails, coverage that doesn't exist).
- Helicone#3, Helicone#5, Helicone#6 strengthen the compatibility guarantee but aren't hard blockers.
- Helicone#4 and Helicone#7 are minor.
See inline comments for detailed explanations and suggested fixes.
MatthiasHowellYopp
left a comment
There was a problem hiding this comment.
Review: Request Changes
⚠️ CI status:claude-reviewFAIL,Jawn Valkey Compatibility TestsPENDING. Other checks pass.
7 warnings, 2 nits. Two blocking themes:
- Unpinned image tag (findings 1, 5, 7) — undermines reproducibility of the test signal this workflow provides
- Docker Compose race (finding 4) — operators following the guide will hit intermittent startup failures
The PR is well-scoped (zero application code changes) and the test coverage is thorough. Addressing the warnings would make this ready to merge.
ac3667e to
7a65ef6
Compare
Jonathan-Improving
left a comment
There was a problem hiding this comment.
QA Review — PR #1: Official Valkey Backend Support
Verdict: COMMENT (no blocking issues — 8 Medium suggestions, 2 Low suggestions)
Summary
Solid PR. The CI correctly validates TLS connectivity against Valkey 7.2. Tests meaningfully exercise production code for 3 of 5 subsystems and accurately test the command surface for the other 2. Documentation is well-structured with an honest "What's Verified" table.
Key findings (8 Medium):
- Docs overclaim ("work identically" vs. "commands verified for 2 subsystems")
- Flaky 30ms/100ms timing margins in CI assertions
- KEYS command in cleanup (O(n) + copy-paste risk pattern)
- 18x non-null assertions without runtime guard
- CI actions not SHA-pinned
- No job-level timeout
- God test file (560 lines, 7 describe blocks, shared singleton)
- chmod 644 on private key (should be 600 for key, 644 for cert)
Suggestions (2 Low):
- --forceExit masks potential connection leaks
- describeIfRedis skips 4 suites silently with no output
Positives
- Production singleton used (no mocking) — validates real TLS connection behavior
- describeIfRedis pattern correctly gates integration tests while allowing unit tests to run freely
- TLS-only Valkey config (--port 0) matches production behavior
- NODE_EXTRA_CA_CERTS is the correct trust mechanism (no app code modification needed)
- Honest "What's Verified" table distinguishing subsystem-imported from command-verified
- Proper afterAll cleanup with REDIS_HOST guard
- Random suffixes per describe block prevent cross-test interference
0649378 to
e1ccede
Compare
- Add integration test validating all Redis command patterns against Valkey - Add GitHub Actions workflow running Jawn tests against valkey - Add self-hosting documentation for Valkey as a supported backend - Zero application code changes, ioredis connects to Valkey transparently Signed-off-by: Riley Desserre <riley.desserre@improving.com>
e1ccede to
2be4415
Compare
Ticket
Helicone#5753
Component/Service
What part of Helicone does this affect?
Type of Change
Deployment Notes
No application code is modified. Operators already on Valkey continue working as-is. Operators on Redis are completely unaffected. For new Valkey adopters, the only change is pointing REDIS_HOST at their Valkey endpoint.
Extra Notes
Context
Helicone's Redis dependency in Jawn works with Valkey transparently (all commands predate the fork), but this compatibility is undocumented, untested in CI, and invisible to operators evaluating Helicone's stack. Two of three closest competitors (LangFuse, LangSmith) already officially support Valkey. This PR adds tests, CI, and documentation confirming what already works.
The five subsystems that touch Redis (KV cache, proxy rate limiter, HTTP API rate limiter, usage-limit cache, encrypted provider key/auth cache) all share a single ioredis client and issue only core Redis commands present since Redis 2.x, all fully supported in Valkey.