feat(state): IoRedisStateAdapter for cross-runtime interop (#71)#73
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a token prefix parameter to Redis lock tokens and introduces an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the IoRedisStateAdapter and adds a token_prefix parameter to the RedisStateAdapter to facilitate cross-runtime Redis sharing with TypeScript-based deployments. The changes include updates to the lock token generation logic, documentation, and the addition of comprehensive unit tests. Feedback identifies an inaccuracy in the new docstring regarding Redis command behavior and suggests updating the create_redis_state factory function to support the new parameter.
| Enables migrations like "drain TS, then flip Python on" with tokens | ||
| identifiable by runtime-of-origin at a glance when inspecting | ||
| ``KEYS chat:lock:*`` in Redis. |
There was a problem hiding this comment.
The docstring mentions that tokens are identifiable when inspecting KEYS chat:lock:*. However, the KEYS command in Redis only returns the names of the keys, not their values (where the tokens reside). To see the tokens, one would need to use GET on those keys. Additionally, since the default key_prefix is chat-sdk, the example pattern would more accurately be chat-sdk:lock:*.
There was a problem hiding this comment.
Addressed in 130064f — docstring reworded to describe reading the lock value via GET chat-sdk:lock:<thread_id> plus mention of logs/trace IDs where tokens surface on release/extend failures. Thanks.
| def __init__(self, *, token_prefix: str = "ioredis", **kwargs: Any) -> None: | ||
| super().__init__(token_prefix=token_prefix, **kwargs) |
There was a problem hiding this comment.
The create_redis_state factory function (located at the end of this file) should be updated to accept and forward the new token_prefix parameter to the RedisStateAdapter constructor. This ensures consistency between the class and its factory method, allowing users of the factory to leverage the new parameterization.
There was a problem hiding this comment.
Addressed in 43f4f40 — create_redis_state() now accepts and forwards token_prefix (and tests cover default + override). Thanks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_state_redis.py (1)
675-688: Use non-blocking sleep in async test.Line 686 uses
time.sleep(...)inside an async test; useawait asyncio.sleep(...)instead to avoid blocking the event loop.Proposed fix
+import asyncio import time @@ - time.sleep(0.15) + await asyncio.sleep(0.15)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_state_redis.py` around lines 675 - 688, In test_extend_lock_works_with_custom_prefix replace the blocking time.sleep call with a non-blocking await asyncio.sleep to avoid blocking the event loop: update the test to use await asyncio.sleep(0.15) (and add an import for asyncio at the top of the test file if missing). This change keeps the test async while still waiting past the original TTL tested by RedisStateAdapter.acquire_lock and RedisStateAdapter.extend_lock so the subsequent acquire_lock call behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_state_redis.py`:
- Around line 675-688: In test_extend_lock_works_with_custom_prefix replace the
blocking time.sleep call with a non-blocking await asyncio.sleep to avoid
blocking the event loop: update the test to use await asyncio.sleep(0.15) (and
add an import for asyncio at the top of the test file if missing). This change
keeps the test async while still waiting past the original TTL tested by
RedisStateAdapter.acquire_lock and RedisStateAdapter.extend_lock so the
subsequent acquire_lock call behaves correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbb9cd53-0552-4611-a3b1-6d61a2ca3ddb
📒 Files selected for processing (4)
CHANGELOG.mdsrc/chat_sdk/state/__init__.pysrc/chat_sdk/state/redis.pytests/test_state_redis.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_state_redis.py (1)
705-705: Use non-blocking sleep in async test code.Line 705 uses
time.sleep(0.15)insideasync def, which blocks the event loop. Preferawait asyncio.sleep(0.15).Proposed patch
import time +import asyncio import pytest- time.sleep(0.15) + await asyncio.sleep(0.15)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_state_redis.py` at line 705, Replace the blocking call time.sleep(0.15) inside the async test with a non-blocking await asyncio.sleep(0.15): locate the async test function that currently calls time.sleep(0.15) and change it to await asyncio.sleep(0.15), and if asyncio is not already imported in tests/test_state_redis.py add an import asyncio at the top so the call resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_state_redis.py`:
- Line 705: Replace the blocking call time.sleep(0.15) inside the async test
with a non-blocking await asyncio.sleep(0.15): locate the async test function
that currently calls time.sleep(0.15) and change it to await
asyncio.sleep(0.15), and if asyncio is not already imported in
tests/test_state_redis.py add an import asyncio at the top so the call resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 551b9646-e3a3-4629-ac35-c3ef5791cde4
📒 Files selected for processing (4)
CHANGELOG.mddocs/UPSTREAM_SYNC.mdsrc/chat_sdk/state/redis.pytests/test_state_redis.py
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- docs/UPSTREAM_SYNC.md
Synchronous time.sleep in an async test blocks the event loop. Addresses coderabbit feedback on PR #73. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review verdict: LGTM. Refreshed and reviewed latest head 9ba45e3 against main. No issues found in Redis/IoRedis lock-token prefix interop changes. Verification: tests/test_state_redis.py passed (68 passed) and ruff passed on redis state/tests. Residual risk: tests use MockRedis rather than a live Redis/ioredis integration. Formal GitHub approval is blocked because the authenticated account owns this PR. |
Sweep remaining sync time.sleep calls in async tests (test_memory_state.py + test_state_postgres.py). Followup to #73 which fixed the same hazard in test_state_redis.py. Also removes one redundant sleep flagged by gemini (lock is force-expired on the next line).
Parameterizes RedisStateAdapter.token_prefix and ships IoRedisStateAdapter subclass defaulting to "ioredis_". Enables cross-runtime Redis sharing between TS and Python chat-sdk deployments — tokens compare by full string equality, but now they carry runtime-of-origin identification for observability and graceful migration. Closes #71. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…redis_state - Corrects the observability example in RedisStateAdapter docstring (KEYS only returns key names, not values) - create_redis_state factory now accepts/forwards token_prefix to keep the factory and direct-instantiation paths aligned Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ivergence Upstream uses Math.random().toString(36) (base36, not CSPRNG); we use secrets.token_hex(16) (32 hex chars, CSPRNG). Interop still works via full-string lock-release equality. Renames the shape-test, updates CHANGELOG and UPSTREAM_SYNC.md Non-Parity table to reflect reality instead of claiming byte-for-byte compatibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Synchronous time.sleep in an async test blocks the event loop. Addresses coderabbit feedback on PR #73. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
feat/ioredis-prefix-71
branch
from
9ba45e3 to
ac59151
Compare
1 participant
Summary
RedisStateAdapter.token_prefix(default"redis"; back-compat preserved).IoRedisStateAdapter(RedisStateAdapter)defaulting totoken_prefix="ioredis"so consumers migrating between TS (Vercel Chat onioredis) and Python chat-sdk deployments can share a Redis with runtime-of-origin-identifiable lock tokens.IoRedisStateAdapterfromchat_sdk.state(matches the existingRedisStateAdapterexport pattern — rootchat_sdk.__init__only exposesMemoryStateAdapter, so nothing added there).SET NX PXacquisition, andPEXPIREextend are unchanged; tokens still compare by full string equality.Closes #71.
Intentional divergence from upstream (not byte-for-byte)
The
ioredis_prefix matches upstream, but the token suffix diverges by design:ioredis_{ms}_{secrets.token_hex(16)}— always 32 hex chars, CSPRNG.state-ioredis/state-redis):ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}— base36, ≤13 chars, not CSPRNG.Cross-runtime lock-release still works because
release_lock/extendcompare the full token string and each runtime only releases tokens it issued on acquire. The divergence is observability-only — bytes in Redis and in log lines differ in shape. We intentionally do not regresssecrets.token_hextoMath.random()for cosmetic byte-for-byte parity; CSPRNG quality matters more than a matching log shape. Documented as a row indocs/UPSTREAM_SYNC.mdunder Known Non-Parity.Test plan
uv run ruff check src/ tests/ scripts/uv run ruff format --check src/ tests/ scripts/uv run pyrefly check— 0 errorsuv run python scripts/audit_test_quality.py— 0 hard failuresuv run python scripts/verify_test_fidelity.py— no new missinguv run pytest tests/ --tb=short -q— 3556 passed, 2 skippedTestRedisStateTokenPrefix:redis_prefix preserved (back-compat)token_prefixappliedIoRedisStateAdapter()default =ioredis_IoRedisStateAdapterisaRedisStateAdaptertoken_prefixon the subclass still winstest_ioredis_token_shape_python_diverges_from_upstream— documents the intentional CSPRNG / hex-vs-base36 divergence (test name and docstring call this out explicitly)redis_-prefixed token with the same suffix cannot release anioredis_lock🤖 Generated with Claude Code
Summary by CodeRabbit