fix: bugfix for logout, oncecell for sse

[parmesant]

logout flow incorrectly assumed the oidc client to always be present

shifted sse handler from lazy to oncecell

Fixes #XXXX.

Description

---

This PR has:

• been tested to ensure log ingestion and log query works.
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added documentation for new or modified features or behaviors.

Summary by CodeRabbit

• Refactor
  • Simplified internal handling for authentication and real-time event components to reduce nesting and streamline initialization/access.
  • Consolidated initialization paths so clients and event broadcasters are instantiated deterministically when configured.
  • Preserved public behavior and APIs; no changes to external signatures or client-facing flows.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This change replaces Lazy statics with OnceCell for the SSE handler and removes the Option wrapper from the OIDC client OnceCell, updating call sites to use guarded retrievals and simplified get() patterns; token-refresh awaits the read lock before refresh. (≈40 words)

Changes

Cohort / File(s)	Summary
SSE handler src/sse/mod.rs, src/alerts/mod.rs	SSE_HANDLER changed from Lazy<Arc<Broadcaster>> to OnceCell<Arc<Broadcaster>>; initialization now via get_or_init(...); alert broadcasts obtain SSE_HANDLER.get() and guard before calling handler.broadcast(...).
OIDC client type src/handlers/http/modal/mod.rs	OIDC_CLIENT changed from OnceCell<Option<Arc<RwLock<GlobalClient>>>> to OnceCell<Arc<RwLock<GlobalClient>>>; initialization closures updated to return the Arc directly and removed None branch.
OIDC call sites & middleware src/handlers/http/oidc.rs, src/handlers/http/middleware.rs	Call sites simplified to use OIDC_CLIENT.get() directly; middleware token refresh now awaits the read lock (.read().await) before calling refresh_token(...).await; logout/login retrievals simplified with fewer nested matches.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat: SSE #1487 — Prior SSE refactor introducing an SSE_HANDLER static and broadcast usages that align with these changes.
• bugfix for oidc #1272 — Related changes to modal/OIDC initialization patterns and OnceCell usage.

Suggested labels

for next release

Suggested reviewers

• nikhilsinhaparseable

Poem

🐰 I hopped through globals, light and well,

Swapped Lazy for OnceCell's shell,
Options tucked were set aside,
Cleaner paths where functions glide,
A little hop — the code says swell!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	The description identifies the main bug (logout flow assuming OIDC client presence) and the SSE handler refactor, but largely leaves the template sections blank without detailed explanations or proper issue reference.	Replace placeholder 'Fixes #XXXX' with actual issue number, expand description section with rationale for changes, and check the verification checklist items or explain why they cannot be completed.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title mentions two key changes (logout bugfix and oncecell for sse) that align with the actual modifications across multiple files, though it could be more descriptive.

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/handlers/http/oidc.rs (1)

83-83: Consider simplifying the OIDC client retrieval.

The .map(|c| c.as_ref()) is unnecessary here. Since Arc implements Deref, you can use the &Arc<RwLock<GlobalClient>> directly with .read().await. However, this doesn't affect correctness.

🔎 Suggested simplification

-    let oidc_client = OIDC_CLIENT.get().map(|c| c.as_ref());
+    let oidc_client = OIDC_CLIENT.get();

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cab9fc0 and 6ff713a.

📒 Files selected for processing (5)

• src/alerts/mod.rs
• src/handlers/http/middleware.rs
• src/handlers/http/modal/mod.rs
• src/handlers/http/oidc.rs
• src/sse/mod.rs

🧰 Additional context used

🧬 Code graph analysis (1)

src/handlers/http/oidc.rs (2)

src/utils/actix.rs (2)

• extract_session_key_from_req (51-71)
• req (31-31)

src/handlers/http/modal/mod.rs (1)

• client (75-77)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: Build Default aarch64-apple-darwin
• GitHub Check: Build Default x86_64-unknown-linux-gnu
• GitHub Check: Build Default aarch64-unknown-linux-gnu
• GitHub Check: Build Default x86_64-pc-windows-msvc
• GitHub Check: Build Kafka x86_64-unknown-linux-gnu
• GitHub Check: Build Kafka aarch64-apple-darwin
• GitHub Check: coverage
• GitHub Check: Quest Smoke and Load Tests for Standalone deployments
• GitHub Check: Quest Smoke and Load Tests for Distributed deployments

🔇 Additional comments (9)

src/sse/mod.rs (2)

29-29: LGTM! OnceCell is a good choice for deferred initialization.

The switch from Lazy to OnceCell ensures the Broadcaster (and its ping loop) is only created when the first SSE client actually connects, improving resource efficiency for deployments that don't use SSE.

Also applies to: 39-39

---

177-180: LGTM! Proper initialization pattern.

Using get_or_init(Broadcaster::create) ensures thread-safe, lazy initialization exactly when needed. The new_client call is correctly chained after initialization.

src/alerts/mod.rs (1)

637-639: LGTM! Proper guard for OnceCell access.

The SSE_HANDLER.get() pattern correctly handles the case where no SSE clients have connected yet (and thus the handler isn't initialized). Silently skipping the broadcast when the handler is absent is appropriate since there are no clients to receive the message anyway.

src/handlers/http/oidc.rs (2)

148-175: LGTM! This fixes the logout bug mentioned in the PR.

The logout flow now correctly handles the case where OIDC is not configured. Previously, the code assumed OIDC_CLIENT was always present, which caused issues. The if let Some(client) guard ensures graceful handling when OIDC is not set up.

---

179-184: LGTM! Proper error handling for missing OIDC client.

Returning Unauthorized when the OIDC client is not configured is appropriate for the reply_login callback endpoint, as this endpoint should only be reachable when OIDC is configured.

src/handlers/http/modal/mod.rs (2)

63-63: LGTM! Good type simplification.

Removing the Option wrapper from inside OnceCell is a cleaner design. Now the semantics are clear: OIDC_CLIENT.get() returns None if OIDC is not configured, and Some(...) if it is. This eliminates the redundant Option<Option<...>> pattern.

---

116-121: LGTM! Proper conditional initialization.

The OIDC client is only initialized when configuration is present. Using get_or_init ensures idempotent, thread-safe initialization even if start were called multiple times.

src/handlers/http/middleware.rs (2)

185-185: LGTM! Simplified OIDC client retrieval.

The direct OIDC_CLIENT.get() call is cleaner than the previous nested Option handling, aligning with the type change in modal/mod.rs.

---

203-220: LGTM! Correct async RwLock handling for token refresh.

The client.read().await.client().refresh_token(...) chain correctly acquires the read lock before accessing the client. Error handling properly removes the session and returns Unauthorized on refresh failure.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff713a and 8aedf5e.

📒 Files selected for processing (5)

• src/alerts/mod.rs
• src/handlers/http/middleware.rs
• src/handlers/http/modal/mod.rs
• src/handlers/http/oidc.rs
• src/sse/mod.rs

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-24T11:09:21.781Z

Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.

Applied to files:

• src/alerts/mod.rs

🧬 Code graph analysis (1)

src/handlers/http/oidc.rs (2)

src/utils/actix.rs (2)

• extract_session_key_from_req (51-71)
• req (31-31)

src/handlers/http/modal/mod.rs (1)

• client (75-77)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: Quest Smoke and Load Tests for Standalone deployments
• GitHub Check: Quest Smoke and Load Tests for Distributed deployments
• GitHub Check: coverage
• GitHub Check: Build Default x86_64-pc-windows-msvc
• GitHub Check: Build Default aarch64-unknown-linux-gnu
• GitHub Check: Build Default x86_64-unknown-linux-gnu
• GitHub Check: Build Kafka x86_64-unknown-linux-gnu
• GitHub Check: Build Kafka aarch64-apple-darwin

🔇 Additional comments (10)

src/alerts/mod.rs (1)

637-640: LGTM! SSE broadcast now safely handles uninitialized handler.

The guard pattern correctly checks if the SSE handler exists before broadcasting. This aligns with the OnceCell change where the handler is only initialized when the first SSE client connects.

src/sse/mod.rs (3)

29-29: LGTM! Import updated for OnceCell.

---

39-39: LGTM! SSE handler initialization pattern changed to OnceCell.

The shift from Lazy to OnceCell is appropriate as it provides explicit control over initialization timing and allows checking whether the handler has been initialized.

---

177-180: LGTM! Handler initialization on first client connection.

The get_or_init ensures the broadcaster is created when the first SSE client registers, which is more efficient than eager initialization.

src/handlers/http/modal/mod.rs (2)

63-63: LGTM! OIDC client type simplified.

Removing the Option wrapper from the OnceCell's inner type is cleaner. The OnceCell itself handles presence/absence via .get() returning Option<&T>.

---

120-120: LGTM! Initialization closure simplified.

The closure now directly returns Arc<RwLock<GlobalClient>> instead of wrapping in Some, matching the updated type signature.

src/handlers/http/middleware.rs (1)

185-185: LGTM! Simplified OIDC client retrieval.

src/handlers/http/oidc.rs (3)

83-83: LGTM! Simplified OIDC client retrieval.

The direct .get() call is cleaner than the previous pattern matching approach.

---

149-165: LGTM! Clearer logout endpoint extraction.

The explicit if-let pattern for extracting the logout endpoint improves readability compared to nested pattern matching.

---

180-184: LGTM! Simplified client retrieval with early return.

The explicit if-let with early return for unauthorized is clearer than nested pattern matching.