fix(authenticator): audit drop-metric + wider refresher store-retry (post-merge #1851 review) - #1882
Conversation
…re-retry (post-merge review) Two CodeRabbit findings from constructorfabric#1851 (posted after the rate-limit reset, once the PR had merged): - audit.rs: the serde_json::to_vec failure branch dropped the event silently (bare `continue`) — unlike every other drop path it didn't bump auth_audit_dropped_total or log. Now it counts + warns. - refresher.rs: the post-grant store-retry was only 3×200ms (~400ms), too short for a realistic Redis failover/blip — a store failure after the IdP rotated the grant would then false-logout the session. Widened to exponential backoff (200ms→3.2s, ~6s over 6 attempts), still well under the 30s per-session lock TTL. Ref: constructorfabric#1583, constructorfabric#1851 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 32 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: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAudit serialization failures now increment drop metrics and emit warnings. Refresh-token persistence retries use a configurable attempt count, exponential backoff, and final-attempt error logging. ChangesAudit drop observability
Refresh token persistence retries
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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: 3
🤖 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 `@src/backend/services/authenticator/src/audit.rs`:
- Around line 216-224: Update the description of auth_audit_dropped_total to
explicitly include serialization failures alongside queue-full and delivery
failures, preserving the existing metric name and other description details.
- Line 222: Update the tracing::warn! call in the audit event serialization
failure path to include the documented target "audit". Preserve the existing
error, action, and message fields so dropped serialization events enter the
audit log stream.
In `@src/backend/services/authenticator/src/refresher.rs`:
- Around line 40-43: Bound the Redis operations in the refresh lock flow around
store_idp_refresh and unlock_session_refresh so their worst-case duration,
including STORE_RETRY_ATTEMPTS backoff, remains below the 30-second lock TTL.
Configure explicit client/session timeouts or apply an equivalent timeout to the
lock-holding flow, and ensure unlock_session_refresh releases the lock only when
the current owner token matches.
🪄 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: d545fe91-3eb4-493d-ae75-2ed4724f718c
📒 Files selected for processing (2)
src/backend/services/authenticator/src/audit.rssrc/backend/services/authenticator/src/refresher.rs
- audit: count serialization failures in the dropped-counter description and route all three drop warnings to the "audit" tracing target so lost events still surface in the audit log stream. - session: make the per-session refresh lock owner-safe — lock returns a unique owner token stored as the lock value; unlock compare-and-dels via Lua so a worker never clears a lock a later worker re-acquired. - refresher: bound the lock-holding critical section (grant + store retries) to below the lock TTL via tokio timeout, so the flow can never outlive the lock and let a second worker burn the same one-time grant. Refs constructorfabric#1583 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
2 participants
Follow-up to the merged step-10 PR #1851: addresses the two CodeRabbit "Major" findings it posted after its review rate-limit reset (once #1851 had already merged), so they weren't fixed in-line.
1. Audit — serialization failures bypassed the dropped-events metric (
audit.rs)The
serde_json::to_vec(&env)failure branch did a barecontinue— unlike the queue-full and delivery-failure paths, it neither incrementedauth_audit_dropped_totalnor logged. A malformed event vanished without a trace. Now it counts + warns like the other drop paths.2. Refresher — post-grant store-retry window too short (
refresher.rs)The retry that protects against a Redis failure after the IdP has already rotated the grant (losing the new token → false logout on the next attempt) was only 3×200ms (~400ms) — shorter than a realistic Redis failover/blip. Widened to exponential backoff (200ms→3.2s, ~6s over
STORE_RETRY_ATTEMPTS = 6), still well under the 30s per-session lock TTL so the lock/permit is never held past it.Verified: clippy clean, authenticator unit tests pass.
Ref: #1583, #1851
🤖 Generated with Claude Code
Summary by CodeRabbit