diff --git a/CHANGELOG.md b/CHANGELOG.md index 56775cc..a4c75bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,24 @@ ## Unreleased +### New public APIs + +- **`IoRedisStateAdapter`**: `RedisStateAdapter` subclass defaulting to the + `ioredis_` lock-token prefix used by upstream Vercel Chat's `ioredis`-backed + state. Enables cross-runtime Redis sharing between TS and Python chat-sdk + deployments during migrations. Closes #71. + Note: the token *shape* after the prefix diverges intentionally — Python + emits `ioredis_{ms}_{hex32}` (`secrets.token_hex(16)`, CSPRNG) whereas + upstream emits `ioredis_{ms}_{base36<=13}` (`Math.random().toString(36)`, + not CSPRNG). Lock-release still works across runtimes because each + runtime generates its own token on acquire and `release_lock` / `extend` + compare the full token string — the divergence is observability-only + (log lines, bytes-in-Redis), not a functional incompatibility. We will + not regress to `Math.random()` for cosmetic byte-for-byte parity. +- **`RedisStateAdapter(token_prefix=...)`**: new `token_prefix` kwarg + (default `"redis"`). Parameterizes the lock-token prefix for observability + and interop. + ### Test hygiene - Sweep remaining `time.sleep` → `await asyncio.sleep` in async tests diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 2600324..9ea2e47 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -458,6 +458,7 @@ stay explicit instead of being rediscovered in code review. | Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | | `SlackAdapter.current_token` / `current_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | Not exposed (`getToken()` is private on the TS `SlackAdapter`) | Python-only addition (issue #47). Downstream code that calls Slack Web APIs from inside a handler — email resolution, user profile fetches, reaction bookkeeping — otherwise depends on underscore-prefixed helpers. | | `ConcurrencyConfig.max_concurrent` | Enforced via `asyncio.Semaphore` in the `"concurrent"` strategy path; rejects non-integer or `<= 0` values, and rejects any non-`None` `max_concurrent` paired with a non-`"concurrent"` strategy | Accepted into the config type with docstring "Default: Infinity" but never read (3 writes, 0 reads) | Silent correctness bug upstream — consumers setting `max_concurrent=N` with `strategy="concurrent"` reasonably expect an N-way bound on in-flight handlers. We honor the documented contract via a semaphore and fail-fast on misconfiguration so it's never silent. `max_concurrent=None` stays compatible with every strategy (unbounded default). | +| Redis lock token format | `{token_prefix}_{ms}_{secrets.token_hex(16)}` — always 32 hex chars, CSPRNG-sourced | `ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}` — base36, ≤13 chars, **not** CSPRNG | Interop via `IoRedisStateAdapter(token_prefix="ioredis")` still works for lock-release (release/extend compare by full-string equality, and each runtime only releases what it issued), but the token byte-shape diverges. Intentional — CSPRNG should not be regressed to `Math.random()` for cosmetic byte-for-byte compatibility. | ### Platform-specific gaps diff --git a/src/chat_sdk/state/__init__.py b/src/chat_sdk/state/__init__.py index 2511271..20caed2 100644 --- a/src/chat_sdk/state/__init__.py +++ b/src/chat_sdk/state/__init__.py @@ -2,9 +2,14 @@ from chat_sdk.state.memory import MemoryStateAdapter, create_memory_state from chat_sdk.state.postgres import PostgresStateAdapter, create_postgres_state -from chat_sdk.state.redis import RedisStateAdapter, create_redis_state +from chat_sdk.state.redis import ( + IoRedisStateAdapter, + RedisStateAdapter, + create_redis_state, +) __all__ = [ + "IoRedisStateAdapter", "MemoryStateAdapter", "PostgresStateAdapter", "RedisStateAdapter", diff --git a/src/chat_sdk/state/redis.py b/src/chat_sdk/state/redis.py index c71448f..0dcf686 100644 --- a/src/chat_sdk/state/redis.py +++ b/src/chat_sdk/state/redis.py @@ -26,8 +26,8 @@ # --------------------------------------------------------------------------- -def _generate_token() -> str: - return f"redis_{int(time.time() * 1000)}_{secrets.token_hex(16)}" +def _generate_token(prefix: str = "redis") -> str: + return f"{prefix}_{int(time.time() * 1000)}_{secrets.token_hex(16)}" # --------------------------------------------------------------------------- @@ -88,6 +88,14 @@ class RedisStateAdapter: across multiple server instances. Implements the full :class:`~chat_sdk.types.StateAdapter` protocol. + + Lock tokens are generated with the shape ``{token_prefix}_{ms}_{hex16}``. + The prefix defaults to ``"redis"``, matching this port's historical + behavior. Set ``token_prefix="ioredis"`` (or use :class:`IoRedisStateAdapter`) + when sharing a Redis with a TypeScript Vercel Chat deployment whose + ``ioredis``-backed state emits ``ioredis_`` tokens. Release and extend + semantics compare the full token string, so the prefix is an observability + and migration aid, not a security boundary. """ def __init__( @@ -96,8 +104,10 @@ def __init__( url: str | None = None, client: Any | None = None, key_prefix: str = "chat-sdk", + token_prefix: str = "redis", ) -> None: self._key_prefix = key_prefix + self._token_prefix = token_prefix self._connected = False self._connect_lock = asyncio.Lock() @@ -162,7 +172,7 @@ async def is_subscribed(self, thread_id: str) -> bool: async def acquire_lock(self, thread_id: str, ttl_ms: int) -> Lock | None: self._ensure_connected() - token = _generate_token() + token = _generate_token(self._token_prefix) lock_key = self._key("lock", thread_id) # SET NX PX for atomic lock acquisition @@ -341,6 +351,41 @@ def _ensure_connected(self) -> None: raise StateNotConnectedError("RedisStateAdapter") +# --------------------------------------------------------------------------- +# ioredis-compatible subclass +# --------------------------------------------------------------------------- + + +class IoRedisStateAdapter(RedisStateAdapter): + """:class:`RedisStateAdapter` variant that emits ``ioredis_``-prefixed lock tokens. + + Use this when sharing a Redis with a TypeScript Vercel Chat deployment whose + ``ioredis``-backed state adapter writes ``ioredis_``-prefixed lock tokens. + Behavior is otherwise identical to :class:`RedisStateAdapter` — same Lua + scripts, same key layout, same ``SET NX PX`` acquisition, same ``PEXPIRE`` + extend. Tokens compare by full string equality, so the prefix is an + observability and migration aid for cross-runtime Redis sharing, not a + security boundary. + + Note: the token suffix shape diverges intentionally. Python emits + ``ioredis_{ms}_{secrets.token_hex(16)}`` (32 hex chars, CSPRNG), whereas + upstream emits ``ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}`` + (base36, ≤13 chars, **not** CSPRNG). Interop for lock-release is + unaffected — each runtime issues and releases its own tokens and the + comparison is by full string equality — but the bytes in Redis and in + log lines differ. We do not regress to ``Math.random()`` for cosmetic + byte-for-byte parity; see ``docs/UPSTREAM_SYNC.md`` Known Non-Parity. + + Enables migrations like "drain TS, then flip Python on" with tokens + identifiable by runtime-of-origin at a glance — e.g. reading a lock's + value via ``GET chat-sdk:lock:``, or in any logs/trace IDs + where the token is surfaced on release/extend failures. + """ + + def __init__(self, *, token_prefix: str = "ioredis", **kwargs: Any) -> None: + super().__init__(token_prefix=token_prefix, **kwargs) + + # --------------------------------------------------------------------------- # Factory # --------------------------------------------------------------------------- @@ -351,10 +396,20 @@ def create_redis_state( url: str | None = None, client: Any | None = None, key_prefix: str = "chat-sdk", + token_prefix: str = "redis", ) -> RedisStateAdapter: """Create a new Redis state adapter. Either provide a ``url`` or an existing ``client``. If neither is given, the ``REDIS_URL`` environment variable is used. + + ``token_prefix`` defaults to ``"redis"`` to match + :class:`RedisStateAdapter`'s default; pass ``"ioredis"`` (or instantiate + :class:`IoRedisStateAdapter`) for TS interop. """ - return RedisStateAdapter(url=url, client=client, key_prefix=key_prefix) + return RedisStateAdapter( + url=url, + client=client, + key_prefix=key_prefix, + token_prefix=token_prefix, + ) diff --git a/tests/test_state_redis.py b/tests/test_state_redis.py index 65eedc4..b651e55 100644 --- a/tests/test_state_redis.py +++ b/tests/test_state_redis.py @@ -8,12 +8,17 @@ from __future__ import annotations +import asyncio import time import pytest from chat_sdk.errors import StateNotConnectedError -from chat_sdk.state.redis import RedisStateAdapter +from chat_sdk.state.redis import ( + IoRedisStateAdapter, + RedisStateAdapter, + create_redis_state, +) from chat_sdk.types import Lock, QueueEntry # ============================================================================ @@ -374,7 +379,7 @@ async def test_delete_nonexistent_is_noop(self, redis_state: RedisStateAdapter): @pytest.mark.asyncio async def test_set_with_ttl_expires(self, redis_state: RedisStateAdapter): await redis_state.set("key", "value", ttl_ms=1) - time.sleep(0.005) + await asyncio.sleep(0.005) assert await redis_state.get("key") is None @pytest.mark.asyncio @@ -385,7 +390,7 @@ async def test_set_with_ttl_available_before_expiry(self, redis_state: RedisStat @pytest.mark.asyncio async def test_set_without_ttl_never_expires(self, redis_state: RedisStateAdapter): await redis_state.set("key", "value") - time.sleep(0.005) + await asyncio.sleep(0.005) assert await redis_state.get("key") == "value" @@ -414,13 +419,13 @@ async def test_set_if_not_exists_when_exists(self, redis_state: RedisStateAdapte async def test_set_if_not_exists_with_ttl(self, redis_state: RedisStateAdapter): result = await redis_state.set_if_not_exists("key", "value", ttl_ms=1) assert result is True - time.sleep(0.005) + await asyncio.sleep(0.005) assert await redis_state.get("key") is None @pytest.mark.asyncio async def test_set_if_not_exists_after_expired_key(self, redis_state: RedisStateAdapter): await redis_state.set("key", "old", ttl_ms=1) - time.sleep(0.005) + await asyncio.sleep(0.005) result = await redis_state.set_if_not_exists("key", "new") assert result is True assert await redis_state.get("key") == "new" @@ -453,7 +458,7 @@ async def test_acquire_lock_fails_when_held(self, redis_state: RedisStateAdapter async def test_acquire_lock_succeeds_after_expiry(self, redis_state: RedisStateAdapter): lock1 = await redis_state.acquire_lock("thread-1", 1) assert lock1 is not None - time.sleep(0.005) + await asyncio.sleep(0.005) lock2 = await redis_state.acquire_lock("thread-1", 30_000) assert lock2 is not None assert lock2.thread_id == "thread-1" @@ -490,7 +495,7 @@ async def test_extend_lock(self, redis_state: RedisStateAdapter): result = await redis_state.extend_lock(lock, 60_000) assert result is True - time.sleep(0.15) + await asyncio.sleep(0.15) # Lock still held because we extended lock2 = await redis_state.acquire_lock("thread-1", 30_000) assert lock2 is None @@ -508,7 +513,7 @@ async def test_extend_lock_wrong_token_fails(self, redis_state: RedisStateAdapte async def test_extend_lock_after_expiry_fails(self, redis_state: RedisStateAdapter): lock = await redis_state.acquire_lock("thread-1", 1) assert lock is not None - time.sleep(0.005) + await asyncio.sleep(0.005) result = await redis_state.extend_lock(lock, 60_000) assert result is False @@ -542,6 +547,194 @@ async def test_independent_locks_per_thread(self, redis_state: RedisStateAdapter assert lock1.token != lock2.token +# ============================================================================ +# Lock token prefix: parameterization + IoRedisStateAdapter interop +# ============================================================================ + + +class TestRedisStateTokenPrefix: + """Lock token prefix parameterization for TS/Python Redis interop (#71).""" + + @pytest.mark.asyncio + async def test_default_prefix_is_redis(self, mock_redis: MockRedis): + """Default RedisStateAdapter keeps the historical 'redis_' prefix (back-compat).""" + adapter = RedisStateAdapter(client=mock_redis, key_prefix="test") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("redis_") + assert not lock.token.startswith("ioredis_") + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_custom_prefix_applied(self, mock_redis: MockRedis): + """token_prefix='custom' emits tokens with exactly that prefix + underscore.""" + adapter = RedisStateAdapter(client=mock_redis, key_prefix="test", token_prefix="custom") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("custom_") + # And it must not accidentally match the default. + assert not lock.token.startswith("redis_") + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_ioredis_subclass_default_prefix(self, mock_redis: MockRedis): + """IoRedisStateAdapter() defaults to the 'ioredis_' prefix for TS interop.""" + adapter = IoRedisStateAdapter(client=mock_redis, key_prefix="test") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("ioredis_") + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_ioredis_subclass_is_redis_adapter(self, mock_redis: MockRedis): + """IoRedisStateAdapter is a RedisStateAdapter so `isinstance` checks pass.""" + adapter = IoRedisStateAdapter(client=mock_redis, key_prefix="test") + assert isinstance(adapter, RedisStateAdapter) + + @pytest.mark.asyncio + async def test_ioredis_subclass_prefix_override(self, mock_redis: MockRedis): + """Explicit token_prefix on IoRedisStateAdapter wins over the 'ioredis' default.""" + adapter = IoRedisStateAdapter(client=mock_redis, key_prefix="test", token_prefix="override") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("override_") + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_ioredis_token_shape_python_diverges_from_upstream(self, mock_redis: MockRedis): + """ioredis_ token shape is an intentional Python-first divergence. + + Upstream (`state-ioredis`/`state-redis`) emits + ``ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}`` + -- base36, variable length (up to 13 chars, fewer if Math.random()'s + decimal has trailing zeros), and **not** from a CSPRNG. + + We intentionally diverge to ``ioredis_{ms}_{secrets.token_hex(16)}`` + -- always 32 hex characters, CSPRNG-sourced. Lock-release still works + cross-runtime because each runtime issues its own token on acquire + and release/extend compare the token by full string equality; the + divergence is cosmetic (log-line shape + bytes observed in Redis). + + Do not regress this to ``Math.random()`` for byte-for-byte parity -- + CSPRNG quality is not worth trading for cosmetic compatibility. + """ + adapter = IoRedisStateAdapter(client=mock_redis, key_prefix="test") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + parts = lock.token.split("_") + # ["ioredis", "", ""] -- Python shape, NOT upstream's base36. + assert len(parts) == 3 + assert parts[0] == "ioredis" + assert parts[1].isdigit() + # secrets.token_hex(16) -> 32 hex characters (upstream emits <=13 base36 chars). + assert len(parts[2]) == 32 + int(parts[2], 16) # raises if not hex + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_release_lock_works_with_custom_prefix(self, mock_redis: MockRedis): + """Release lock compares full string — custom prefix must still release.""" + adapter = RedisStateAdapter(client=mock_redis, key_prefix="test", token_prefix="ioredis") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("ioredis_") + + # Release with the correct (ioredis-prefixed) token succeeds. + await adapter.release_lock(lock) + + # Proof the key was actually deleted — reacquire succeeds. + lock2 = await adapter.acquire_lock("thread-1", 30_000) + assert lock2 is not None + assert lock2.token.startswith("ioredis_") + assert lock2.token != lock.token + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_release_lock_wrong_prefix_is_noop(self, mock_redis: MockRedis): + """A token with a wrong prefix but otherwise-similar shape must not release.""" + adapter = RedisStateAdapter(client=mock_redis, key_prefix="test", token_prefix="ioredis") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + + # Fabricate a "redis_"-prefixed token with same suffix. + suffix = lock.token[len("ioredis_") :] + forged = Lock( + thread_id="thread-1", + token=f"redis_{suffix}", + expires_at=lock.expires_at, + ) + await adapter.release_lock(forged) + + # Original lock still held. + collide = await adapter.acquire_lock("thread-1", 30_000) + assert collide is None + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_extend_lock_works_with_custom_prefix(self, mock_redis: MockRedis): + """Extend lock must succeed when the token matches by full string (custom prefix).""" + adapter = RedisStateAdapter(client=mock_redis, key_prefix="test", token_prefix="ioredis") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 100) + assert lock is not None + ok = await adapter.extend_lock(lock, 60_000) + assert ok is True + + # Wait past the original 100ms TTL; extend bumped it to 60s. + await asyncio.sleep(0.15) + contender = await adapter.acquire_lock("thread-1", 30_000) + assert contender is None + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_create_redis_state_default_token_prefix(self, mock_redis: MockRedis): + """create_redis_state() defaults to 'redis' token prefix (back-compat).""" + adapter = create_redis_state(client=mock_redis, key_prefix="test") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("redis_") + assert not lock.token.startswith("ioredis_") + finally: + await adapter.disconnect() + + @pytest.mark.asyncio + async def test_create_redis_state_forwards_token_prefix(self, mock_redis: MockRedis): + """create_redis_state(token_prefix=...) forwards through to the adapter.""" + adapter = create_redis_state(client=mock_redis, key_prefix="test", token_prefix="ioredis") + await adapter.connect() + try: + lock = await adapter.acquire_lock("thread-1", 30_000) + assert lock is not None + assert lock.token.startswith("ioredis_") + finally: + await adapter.disconnect() + + # ============================================================================ # Lists: append with maxLength # ============================================================================ @@ -579,7 +772,7 @@ async def test_append_preserves_order(self, redis_state: RedisStateAdapter): @pytest.mark.asyncio async def test_list_with_ttl_expires(self, redis_state: RedisStateAdapter): await redis_state.append_to_list("key", "a", ttl_ms=1) - time.sleep(0.005) + await asyncio.sleep(0.005) result = await redis_state.get_list("key") assert result == []