Remove Redis keyspace SCANs in preparation for cluster mode - #2139
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 8edb00de40200ef9121739161fb3b77c2aaca80f and 80fdf4c. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR updates authenticator architecture documentation, adds Redis 7 startup validation and writer-side session index cleanup, and changes catalog cache invalidation from keyspace scanning to registry-based flushing. ChangesAuthenticator architecture and session cleanup
Catalog cache registry flushing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/services/authenticator/src/session.rs (1)
401-472: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t rely on this pipeline for Redis Cluster.
create_sessionrunspipe.atomic()acrossasm:session:*,asm:token:*,asm:jwt:*,asm:user_sessions:*,asm:sid_index:*,asm:sub_index:*, and literalasm:idp_refresh_due. Those keys are not in one slot, and Redis Cluster rejects multi-key commands such as MULTI/EXEC that span slots unless the keys satisfy a shared{...}tag. If Cluster is supported, use aredis+cluster://client and a single-namespace pattern with hash tags for all joined keys, or split this write into isolated-slot pipelines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/src/session.rs` around lines 401 - 472, The create_session pipeline must not execute atomic MULTI/EXEC across keys in different Redis Cluster slots. Update the key-generation symbols used by create_session, including user_sessions_key, sid_index_key, sub_index_key, session, token, JWT, and idp_refresh_due keys, to use a shared hash-tag namespace, or split the writes into slot-isolated pipelines while preserving atomicity within each slot. Ensure the Redis client configuration supports the selected Cluster-safe approach.
🧹 Nitpick comments (2)
src/backend/services/analytics/src/infra/cache/catalog_cache.rs (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove documentation comments from service and private implementation items.
src/backend/services/analytics/src/infra/cache/catalog_cache.rs#L34-L36: remove the service module documentation.src/backend/services/analytics/src/infra/cache/catalog_cache.rs#L77-L79: remove the private constant documentation and enforce required behavior with tests.As per coding guidelines, “Use
///documentation comments only on exported items in shared library crates” and “Do not add documentation comments to binaries or services.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/analytics/src/infra/cache/catalog_cache.rs` around lines 34 - 36, Remove the service module documentation at src/backend/services/analytics/src/infra/cache/catalog_cache.rs lines 34-36 and the private constant documentation at lines 77-79. Add tests covering the required behavior currently described by the private constant documentation; no direct documentation changes are needed elsewhere.Source: Coding guidelines
src/backend/services/authenticator/src/session.rs (1)
217-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument and test the Redis version gate.
SessionManager::connectnow fails whenINFOfails or Redis reports a major version below 7, butconnect’s# Errorsstill only mentions malformed URLs and initial connection failures. Re-check the module for the exact test location and add a named test: reject Redis 6 or malformedredis_version:metadata; accept Redis 7.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/src/session.rs` around lines 217 - 241, Update the documentation for SessionManager::connect so its Errors section includes Redis INFO failures and rejection of Redis versions below 7. Add a named test at the module’s existing test location covering Redis 6 and malformed redis_version metadata as rejected cases, plus Redis 7 as an accepted case, reusing the version-gating logic rather than requiring live Redis connections.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/components/backend/authenticator/DESIGN.md`:
- Line 39: Update the inherited decisions TOC entry in DESIGN.md to use the
fragment generated from its matching heading, or add an explicit anchor that
matches the existing `#inherited-decisions-dd-bff--dd-router` target, so the link
resolves and MD051 passes.
In `@src/backend/services/analytics/src/infra/cache/catalog_cache.rs`:
- Around line 390-393: Update the registry handling around the `sadd` call to
use expiry-aware, bounded-retention membership and remove entries when tenant
caches are invalidated or expire. In `flush_all`, process `TENANT_REGISTRY_KEY`
members in bounded batches or via streaming rather than buffering the full set,
and cap concurrent `UNLINK` operations; apply these changes at both
catalog_cache.rs:390-393 and catalog_cache.rs:427-438.
- Around line 440-442: Update flush_all and the corresponding put path so
concurrent rewrites cannot remove a registry membership after a newer payload
write; use a versioned registry entry or an equivalent write/flush barrier, and
ensure SREM only removes entries whose version still matches the enumerated
snapshot. Add an integration test that forces put to occur between UNLINK and
SREM and verifies the key remains registered.
In `@src/backend/services/authenticator/src/janitor.rs`:
- Around line 3-9: Update the janitor metrics and documentation around
janitor_pass so refresh backlog is reported independently from janitor pass
liveness. Add or use a dedicated pass-heartbeat metric emitted on each
successful janitor pass, and stop treating a rising backlog metric as evidence
that no pod is running.
In `@src/backend/services/authenticator/src/session.rs`:
- Around line 825-829: Split the responsibilities currently concentrated in
session.rs into focused Rust modules, keeping one primary noun per file and
limiting each module to roughly 400 lines. Extract Redis connection,
login-state/index handling, session lifecycle, IdP refresh, janitor, and
back-channel logout workers into appropriately named modules; update module
declarations, imports, visibility, and call sites so behavior and public
interfaces remain unchanged.
- Around line 401-419: Document the legacy authenticator-index migration/reset
and the limitation that writer-only EXPIREAT NX/GT cleanup cannot bound
untouched no-TTL indexes or validate existing members. Update the index
write/cleanup paths in src/backend/services/authenticator/src/session.rs:401-419
and 430-472, the related expiry/lifetime note at
src/backend/services/authenticator/src/session.rs:825-829, and the janitor
context at src/backend/services/authenticator/src/janitor.rs:3-9; update the
corresponding design documentation at
docs/components/backend/authenticator/DESIGN.md:447, 770-780, and 955-971.
Preserve the current EXPIREAT behavior while clearly requiring a one-time legacy
asm:* reset or migration.
---
Outside diff comments:
In `@src/backend/services/authenticator/src/session.rs`:
- Around line 401-472: The create_session pipeline must not execute atomic
MULTI/EXEC across keys in different Redis Cluster slots. Update the
key-generation symbols used by create_session, including user_sessions_key,
sid_index_key, sub_index_key, session, token, JWT, and idp_refresh_due keys, to
use a shared hash-tag namespace, or split the writes into slot-isolated
pipelines while preserving atomicity within each slot. Ensure the Redis client
configuration supports the selected Cluster-safe approach.
---
Nitpick comments:
In `@src/backend/services/analytics/src/infra/cache/catalog_cache.rs`:
- Around line 34-36: Remove the service module documentation at
src/backend/services/analytics/src/infra/cache/catalog_cache.rs lines 34-36 and
the private constant documentation at lines 77-79. Add tests covering the
required behavior currently described by the private constant documentation; no
direct documentation changes are needed elsewhere.
In `@src/backend/services/authenticator/src/session.rs`:
- Around line 217-241: Update the documentation for SessionManager::connect so
its Errors section includes Redis INFO failures and rejection of Redis versions
below 7. Add a named test at the module’s existing test location covering Redis
6 and malformed redis_version metadata as rejected cases, plus Redis 7 as an
accepted case, reusing the version-gating logic rather than requiring live Redis
connections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 173f41d3-6e8e-49d5-a0ff-7c0db27b94da
📥 Commits
Reviewing files that changed from the base of the PR and between 153a278 and 8edb00de40200ef9121739161fb3b77c2aaca80f.
📒 Files selected for processing (6)
docs/components/backend/authenticator/DESIGN.mddocs/components/backend/authenticator/PRD.mdsrc/backend/services/analytics/src/domain/catalog/reader.rssrc/backend/services/analytics/src/infra/cache/catalog_cache.rssrc/backend/services/authenticator/src/janitor.rssrc/backend/services/authenticator/src/session.rs
| //! Per-key Redis TTLs remove session records and token mappings; the | ||
| //! login-state live index and refresh-schedule orphans linger until trimmed. | ||
| //! Per-user session indexes are trimmed inline by writers and TTL-bounded, | ||
| //! so the pass issues no keyspace SCAN. One leader (Redis lock, DD-BFF-09 — | ||
| //! same election as the refresher) runs a pass every | ||
| //! `janitor_interval_seconds` (default 30 s) and emits removed/backlog | ||
| //! metrics; a rising backlog means no pod is running passes. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Separate backlog from pass liveness.
janitor_pass counts overdue refresh entries before orphan trimming. A rising backlog can also result from a delayed refresher or repeated IdP failures while the janitor continues to run. Use a separate pass-heartbeat metric for “no pod is running.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/authenticator/src/janitor.rs` around lines 3 - 9, Update
the janitor metrics and documentation around janitor_pass so refresh backlog is
reported independently from janitor pass liveness. Add or use a dedicated
pass-heartbeat metric emitted on each successful janitor pass, and stop treating
a rising backlog metric as evidence that no pod is running.
| /// One janitor pass (DESIGN §4.3): trim expired members from the | ||
| /// login-state index and drop long-overdue orphans from the refresh | ||
| /// schedule (live sessions are re-scheduled by the refresher; an entry | ||
| /// still due after `orphan_grace` has no owner). Per-user session | ||
| /// indexes are trimmed inline by writers and TTL-bounded — no SCAN. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split session.rs into focused modules.
session.rs is over 1,000 lines and mixes Redis connection, login state, session lifecycle, IdP refresh, janitor, and back-channel logout. Move worker and index responsibilities into focused modules.
As per coding guidelines: keep one noun per Rust file and split modules when a module exceeds approximately 400 lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/authenticator/src/session.rs` around lines 825 - 829,
Split the responsibilities currently concentrated in session.rs into focused
Rust modules, keeping one primary noun per file and limiting each module to
roughly 400 lines. Extract Redis connection, login-state/index handling, session
lifecycle, IdP refresh, janitor, and back-channel logout workers into
appropriately named modules; update module declarations, imports, visibility,
and call sites so behavior and public interfaces remain unchanged.
Source: Coding guidelines
8edb00d to
3706d1a
Compare
f1a4b22 to
855ef20
Compare
Redis Cluster has no cluster-wide SCAN cursor, so both backend keyspace scans go away; no SCAN remains anywhere in the backend. Authenticator: create_session trims already-expired members from the per-user session index in its own (single-key, single-slot) pipeline, and the user/sid/sub indexes carry EXPIREAT NX/GT TTLs so abandoned keys self-expire — NX seeds a TTL, GT only ever extends, which also stops a shorter-lived session from cutting a shared index's TTL under a live one. The janitor keeps only its single-key duties (login-state trim, refresh-schedule orphans). SessionManager::connect fails fast on Redis < 7.0 (EXPIREAT NX/GT). Legacy pre-TTL indexes are dropped at upgrade rather than repaired: flush asm:* (users re-login) or accept the TTL-bounded listing/revocation blind spot. Analytics: flush_all walks a cat:v1:tenants registry set instead of SCAN cat:v1:*. UNLINKs stay single-key (a batch could span cluster slots), run concurrently with a bounded in-flight cap, and only the enumerated members are SREMed so a concurrent put's registration is never lost. CI: the coverage job starts a Redis 7 container and runs the cache live tests in a second, serial llvm-cov invocation (flush_all wipes the shared keyspace, so parallel cache tests race each other). Docs: authenticator DESIGN.md updated (janitor scope, key-cleanup and pre-TTL-upgrade notes); DESIGN/PRD references to the deleted API Gateway spec tree removed — inline decision references are bare IDs resolved by the section 5 'Inherited decisions' list. Requires Redis >= 7.0, enforced at boot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
855ef20 to
80fdf4c
Compare
2 participants
Summary
Redis Cluster has no cluster-wide
SCANcursor, so the two keyspace scans in the backend must go before cluster mode can be supported. This PR removes both; after it, no keyspace SCAN remains anywhere in the backend.Authenticator
create_sessiontrims already-expired members from the per-user session index (ZREMRANGEBYSCORE) inside the existing pipeline — same key, so the transaction stays single-slot on a cluster.EXPIREATat the longest member's absolute expiry, appliedNXthenGT(GTtreats a TTL-less key as infinite, soNXseeds it;GTthen only ever extends). Abandoned indexes self-expire instead of waiting for a sweeper.EXPIREATto the sameNX/GTguard — this also fixes a latent issue where a later, shorter-lived session could cut the shared index's TTL under a longer-lived one.asm:user_sessions:*. Metric names are unchanged; removed/backlog now cover only those duties — alert thresholds calibrated on index backlog should be reviewed.SessionManager::connectfails fast on Redis < 7.0 instead of surfacingEXPIREAT NX/GTrejections as a 500 on every login.asm:*at upgrade (users re-login), or accept that pre-upgrade sessions live out their TTL with a possible listing/revocation blind spot.Analytics
flush_allwalks a registry set (cat:v1:tenants,SADD-maintained on everyput) instead ofSCAN cat:v1:*.UNLINKs stay single-key (a batch could span cluster slots) but run concurrently on the multiplexed connection, and the enumerated members are removed with oneSREM— never the whole set, so a concurrentput's registration can't be lost.flush_allduring the first rolling deploy can miss them — the 5-minute per-hash TTL clears them shortly after.Docs
cfs validatepasses on both.Requirements
Redis ≥ 7.0 (
EXPIREATwithNX/GToptions) — enforced at boot.Validation
cargo clippy --all-targetsclean; unit tests 589 passedflush_allisolation test) against Redis 7: passedtests/run-e2e.sh, live Redis + binaries): 9/9 suites passed on the final revision🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Documentation