fix(tokens): claim redis socket tokens atomically#6771
Conversation
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR fixes a TOCTOU race in
Confidence Score: 4/5The production fix is correct and safe to merge; the core atomic-claim logic works as intended and error fallback paths are preserved. The implementation change is straightforward and well-targeted. The retry loop has no iteration cap, which is benign in practice with UUID4 but could cause a hang under unusual Redis proxy behaviour. The new concurrency test carries dead coordination code from the old design, making the test intent misleading without affecting correctness or coverage. tests/units/utils/test_token_manager.py — the new atomicity test contains unused async exists coordination code that should be cleaned up. Important Files Changed
Reviews (1): Last reviewed commit: "fix(tokens): claim redis socket tokens a..." | Re-trigger Greptile |
| redis_values = {} | ||
| exists_calls = 0 | ||
| both_checked = asyncio.Event() | ||
|
|
||
| async def exists(key): | ||
| nonlocal exists_calls | ||
| result = key in redis_values | ||
| exists_calls += 1 | ||
| if exists_calls == 2: | ||
| both_checked.set() | ||
| await both_checked.wait() | ||
| return result | ||
|
|
||
| def set_value(key, value, *, ex, nx=False): | ||
| if nx and key in redis_values: | ||
| return False | ||
| redis_values[key] = value | ||
| return True | ||
|
|
||
| mock_redis.exists.side_effect = exists |
There was a problem hiding this comment.
Dead code from old implementation left in concurrency test
The exists async function, both_checked event, and exists_calls counter are never exercised because the new link_token_to_sid no longer calls redis.exists at all. mock_redis.exists.side_effect = exists is also set but never triggered. These are artefacts from the previous check-then-set design. As written, the test only exercises the sequential path through set_value (a synchronous side effect with no await), which means the event loop does not interleave the two tasks — the test passes, but for a different reason than the comment implies. Removing the dead coordination code and documenting that the test validates the NX-wins-first-caller contract would make the intent clearer.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| while not await self.redis.set( | ||
| self._get_redis_key(token), | ||
| socket_record_data, | ||
| ex=self.token_expiration, | ||
| nx=True, | ||
| ): | ||
| token = new_token = _get_new_token() |
There was a problem hiding this comment.
Unbounded retry loop on persistent Redis False returns
The while not await self.redis.set(..., nx=True) loop has no iteration cap. UUID4 collision is practically impossible, so a legitimate livelock is essentially inconceivable. However, if Redis misbehaves and returns falsy (e.g. due to a proxy that strips the NX response or a mock returning 0 instead of raising), the loop will spin indefinitely without any timeout or circuit-breaker. A guard such as a maximum retry count with an emergency Exception raise would prevent a silent hang in such edge cases.
All Submissions:
Type of change
Description
RedisTokenManager.link_token_to_sidpreviously checked whether a token existedbefore storing its socket record in a separate Redis command. Concurrent workers
could both observe an absent token and then associate it with different sockets,
allowing two connections to share the same client state.
This change claims each token with an atomic Redis
SETusingNXand the existingexpiration. A worker that loses the claim retries with a new UUID, while Redis
errors continue to use the local token-manager fallback.
The regression test coordinates two managers against shared Redis state and verifies
that only one keeps the requested token. It fails against the previous implementation
and passes with the atomic claim.
Testing
uv run pytest tests/units/utils/test_token_manager.py -q(28 passed, 4 skipped)uv run pytest tests/units --cov --no-cov-on-fail --cov-report=(6625 passed,17 skipped, 78.04% coverage)
uv run ruff check .uv run ruff format . --checkuv run pyright reflex tests(0 errors)uv run towncrier check --config pyproject.toml --compare-with origin/mainChanges To Core Features: