src/alerts/target.rs (2)

224-264: Consider adding defensive logging for the Paused state.

The refactoring to use context-based notification config and the new state handling looks good. However, since the comment indicates that evaluations shouldn't run for paused alerts, consider adding a warning log to help diagnose if this case is unexpectedly hit.

-            // do not send out any notifs
-            // (an eval should not have run!)
-            AlertState::Paused => {}
+            // do not send out any notifs
+            // (an eval should not have run!)
+            AlertState::Paused => {
+                warn!("Alert evaluation called for paused alert: {}", context.alert_info.alert_id);
+            }

---

574-582: Remove commented out Silenced state handling code.

Since the Silenced state has been replaced with Paused, the commented code is no longer needed and should be removed for clarity.

-            AlertState::Paused => alert["labels"]["status"] = "paused".into(), // AlertState::Silenced => {
-                                                                               //     alert["labels"]["status"] = "silenced".into();
-                                                                               //     alert["annotations"]["reason"] =
-                                                                               //         serde_json::Value::String(payload.default_silenced_string());
-                                                                               //     // alert["endsAt"] = Utc::now()
-                                                                               //     //     .to_rfc3339_opts(chrono::SecondsFormat::Millis, true)
-                                                                               //     //     .into();
-                                                                               // }
+            AlertState::Paused => alert["labels"]["status"] = "paused".into(),

src/alerts/alert_structs.rs (2)

85-94: Inconsistent status formatting in message strings.

The default_resolved_string uses backticks around not-triggered while default_paused_string doesn't use them around paused. Consider making the formatting consistent.

     pub(crate) fn default_resolved_string(&self) -> String {
-        format!("{} is now `not-triggered` ", self.alert_info.alert_name)
+        format!("{} is now `not-triggered`", self.alert_info.alert_name)
     }
 
     pub(crate) fn default_paused_string(&self) -> String {
         format!(
-            "{} is now `paused`. No more evals will be run till it is `paused`.",
+            "{} is now `paused`. No more evals will be run while it is paused.",
             self.alert_info.alert_name
         )
     }

Also, there's a typo in line 91: "till it is paused" should probably be "while it is paused" or "until it is resumed".

---

96-101: Remove commented out code.

The default_silenced_string method is no longer needed since Silenced state has been replaced with Paused.

-    // fn default_silenced_string(&self) -> String {
-    //     format!(
-    //         "Notifications for {} have been silenced ",
-    //         self.alert_info.alert_name
-    //     )
-    // }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:31-41
Timestamp: 2025-06-18T11:15:10.836Z
Learning: DataFusion's parquet reader defaults to using view types (Utf8View, BinaryView) when reading parquet files via the schema_force_view_types configuration (default: true). This means StringViewArray and BinaryViewArray downcasting is required when processing Arrow arrays from DataFusion parquet operations, even though these types are behind nightly feature flags.

src/alerts/mod.rs (1)

949-953: Critical issue resolved: Early return bug fixed.

This code segment correctly uses continue instead of return Ok(()) when processing paused alerts, which fixes the critical bug identified in previous reviews where loading would stop after the first paused alert.

src/alerts/alert_structs.rs (1)

89-94: Fix typo in paused string message.

Line 91 has a logical error in the message - it says "till it is paused" but should say "till it is resumed" or "unpaused".

-            "{} is now `paused`. No more evals will be run till it is `paused`.",
+            "{} is now `paused`. No more evals will be run till it is `resumed`.",

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:31-41
Timestamp: 2025-06-18T11:15:10.836Z
Learning: DataFusion's parquet reader defaults to using view types (Utf8View, BinaryView) when reading parquet files via the schema_force_view_types configuration (default: true). This means StringViewArray and BinaryViewArray downcasting is required when processing Arrow arrays from DataFusion parquet operations, even though these types are behind nightly feature flags.

src/handlers/livetail.rs (1)

283-283: Do not panic on malformed cookies.

.expect("cookie is parseable") can crash the server if a client sends an invalid Cookie header. Handle parse errors gracefully.

Apply this safer change:

-        .map(|value| value.expect("cookie is parseable"))
+        .filter_map(|res| res.ok())

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

49-51: Avoid panics: replace header/JSON unwraps with fallible parsing

to_str().unwrap() and serde_json::from_str(...).unwrap() on untrusted headers can panic and DoS the service. Handle errors gracefully and log at warn level.

Suggested change (outside the changed hunk, shown for clarity):

if let Some(kinesis_common_attributes) =
    req.request().headers().get(KINESIS_COMMON_ATTRIBUTES_KEY)
{
    if let Ok(attribute_value) = kinesis_common_attributes.to_str() {
        match serde_json::from_str::<Message>(attribute_value) {
            Ok(message) => {
                log_builder = log_builder.with_stream(message.common_attributes.x_p_stream);
            }
            Err(e) => {
                tracing::warn!(
                    "invalid {} header JSON: {e}",
                    KINESIS_COMMON_ATTRIBUTES_KEY
                );
            }
        }
    } else {
        tracing::warn!("non-UTF8 {} header", KINESIS_COMMON_ATTRIBUTES_KEY);
    }
}

src/handlers/livetail.rs (1)

278-289: Avoid intermediate Vec allocation; stream the search.

You can remove the collect and find directly on the iterator to reduce allocations and improve performance.

Apply this refactor:

-    let cookies = header.get_all("cookie");
-    let cookies: Vec<_> = cookies
-        .iter()
-        .filter_map(|value| value.to_str().ok())
-        .flat_map(Cookie::split_parse)
-        .filter_map(|res| res.ok())
-        .collect();
-
-    cookies
-        .into_iter()
-        .find(|cookie| cookie.name() == SESSION_COOKIE_NAME)
+    header
+        .get_all("cookie")
+        .iter()
+        .filter_map(|value| value.to_str().ok())
+        .flat_map(Cookie::split_parse)
+        .filter_map(|res| res.ok())
+        .find(|cookie| cookie.name() == SESSION_COOKIE_NAME)

src/about.rs (1)

155-155: Typo in comment: “Cargol.toml” → “Cargo.toml”

Fix the filename spelling in the comment.

-    // CARGO_PKG_VERSION is set from Cargol.toml file at build time
+    // CARGO_PKG_VERSION is set from Cargo.toml file at build time

src/migration/metadata_migration.rs (1)

79-81: Nit: typo in panic message

"arrray" → "array".

-            panic!("privileges is an arrray")
+            panic!("privileges is an array")

src/catalog/mod.rs (1)

295-296: Nit: variable name typo

"mainfest_file_name" → "manifest_file_name".

-    let mainfest_file_name = manifest_path("").to_string();
-    let path = partition_path(stream_name, lower_bound, upper_bound).join(&mainfest_file_name);
+    let manifest_file_name = manifest_path("").to_string();
+    let path = partition_path(stream_name, lower_bound, upper_bound).join(&manifest_file_name);

src/handlers/http/modal/query/querier_logstream.rs (1)

39-41: Nit: function name typo in import

merge_quried_stats looks misspelled. Consider renaming to merge_queried_stats across the codebase.

I can open a follow-up PR to rename and update call sites.

src/query/stream_schema_provider.rs (1)

175-185: Nit: function name typo

get_hottier_exectuion_plan contains typos (“hottier”, “exectuion”). Consider renaming to get_hot_tier_execution_plan for clarity.

I can generate a patch and a script to update all references safely.

src/alerts/alerts_utils.rs (1)

277-293: Avoid sending empty notification messages

update_alert_state uses Some("".into()) to mark a resolution path.
Because trigger_notif.is_some() is later checked, this still fires a notification – but with an empty body.

Consider using None for “no message” or provide a meaningful “resolved” message instead, e.g.:

-            .update_state(*alert.get_id(), AlertState::NotTriggered, Some("".into()))
+            .update_state(*alert.get_id(), AlertState::NotTriggered, None)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1392
File: src/migration/stream_metadata_migration.rs:303-322
Timestamp: 2025-07-28T17:10:39.448Z
Learning: In Parseable's migration system (src/migration/stream_metadata_migration.rs), each migration function updates the metadata to the current latest format using CURRENT_OBJECT_STORE_VERSION and CURRENT_SCHEMA_VERSION constants, rather than producing incremental versions. For example, v5_v6 function produces v7 format output when these constants are set to "v7", not v6 format.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:351-355
Timestamp: 2025-06-16T05:20:18.593Z
Learning: In the Parseable codebase, arrow files are expected to always have valid creation or modified timestamps as a basic system assumption. The conversion flow uses expect() on file metadata operations to enforce this invariant with fail-fast behavior rather than graceful error handling, as violations represent fundamental system issues that should cause immediate failures.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:832-843
Timestamp: 2025-06-18T06:45:37.070Z
Learning: Stats calculation for parquet files in Parseable is done synchronously during the upload process because files are deleted from staging after upload. This prevents race conditions and ensures stats are calculated while files are still available locally.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.

Learnt from: parmesant
PR: parseablehq/parseable#1347
File: src/handlers/http/query.rs:0-0
Timestamp: 2025-06-18T12:44:31.983Z
Learning: The counts API in src/handlers/http/query.rs does not currently support group_by functionality in COUNT queries, so the hard-coded fields array ["start_time", "end_time", "count"] is appropriate for the current scope.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.

src/alerts/target.rs (1)

381-404: Variable name timeout is misleading

The variable name timeout is confusing since it represents a NotificationConfig. Consider renaming it to better reflect its purpose.

-        let mut timeout = NotificationConfig::default();
+        let mut notification_config = NotificationConfig::default();

         // Default is Infinite in case of alertmanager
         if matches!(value.target, TargetType::AlertManager(_)) {
-            timeout.times = Retry::Infinite
+            notification_config.times = Retry::Infinite
         }

         if let Some(notification_config) = value.notification_config {
             let interval = notification_config.interval.map(|ref interval| *interval);

             if let Some(interval) = interval {
-                timeout.interval = interval
+                notification_config.interval = interval
             }

             if let Some(times) = notification_config.times {
-                timeout.times = Retry::Finite(times)
+                notification_config.times = Retry::Finite(times)
             }
         }

src/alerts/alerts_utils.rs (2)

227-233: Fix compile error: Invalid if let pattern with &&

The pattern if let Some(int_array) = ... && !int_array.is_null(...) is not valid stable Rust syntax. You need to separate the pattern matching from the guard condition.

-} else if let Some(int_array) = column.as_any().downcast_ref::<Int64Array>()
-    && !int_array.is_null(row_index)
-{
-    return int_array.value(row_index) as f64;
+} else if let Some(int_array) = column.as_any().downcast_ref::<Int64Array>() {
+    if !int_array.is_null(row_index) {
+        return int_array.value(row_index) as f64;
+    }
 }

---

247-252: Fix compile error: Invalid if let pattern with &&

Same issue as above - the if let ... && guard pattern needs to be split.

-} else if let Some(float_array) = column.as_any().downcast_ref::<Float64Array>()
-    && !float_array.is_null(row_index)
-{
-    return float_array.value(row_index).to_string();
+} else if let Some(float_array) = column.as_any().downcast_ref::<Float64Array>() {
+    if !float_array.is_null(row_index) {
+        return float_array.value(row_index).to_string();
+    }
 }

src/alerts/mod.rs (1)

830-836: Fix compile error: Cannot sum iterator of Result values

The code attempts to sum an iterator of Result<usize, AlertError> values, but Result doesn't implement the Sum trait. This will cause a compilation error.

-        _ => {
-            // Use inputs() method to get all input plans
-            plan.inputs()
-                .iter()
-                .map(|input| _get_number_of_agg_exprs(input))
-                .sum()
-        }
+        _ => {
+            // Use inputs() method to get all input plans
+            plan.inputs()
+                .iter()
+                .try_fold(0usize, |acc, input| {
+                    _get_number_of_agg_exprs(input).map(|n| acc + n)
+                })
+        }

src/alerts/target.rs (2)

39-43: Consider consolidating import groups for better organization

The imports could be better organized by grouping related items together. Consider restructuring them for improved readability.

 use crate::{
-    alerts::{AlertError, alert_traits::CallableTarget},
+    alerts::{
+        AlertError, AlertState, Context,
+        alert_traits::CallableTarget,
+    },
     parseable::PARSEABLE,
     storage::object_storage::target_json_path,
 };

---

574-581: Remove commented-out code

There's commented-out code that should be removed if it's no longer needed. If this functionality might be needed later, consider tracking it in an issue instead.

-            AlertState::Disabled => alert["labels"]["status"] = "disabled".into(), // AlertState::Silenced => {
-                                                                                   //     alert["labels"]["status"] = "silenced".into();
-                                                                                   //     alert["annotations"]["reason"] =
-                                                                                   //         serde_json::Value::String(payload.default_silenced_string());
-                                                                                   //     // alert["endsAt"] = Utc::now()
-                                                                                   //     //     .to_rfc3339_opts(chrono::SecondsFormat::Millis, true)
-                                                                                   //     //     .into();
-                                                                                   // }
+            AlertState::Disabled => alert["labels"]["status"] = "disabled".into(),

src/alerts/alert_structs.rs (1)

466-474: Consider handling edge cases in get_single_value

The method returns 0.0 when groups is empty, which might mask actual query failures. Consider whether this should return an Option or Result<f64, Error> instead.

     pub fn get_single_value(&self) -> f64 {
         if self.is_simple_query && !self.groups.is_empty() {
             self.groups[0].aggregate_value
         } else {
             0.0
         }
     }

This could be:

pub fn get_single_value(&self) -> Option<f64> {
    if self.is_simple_query && !self.groups.is_empty() {
        Some(self.groups[0].aggregate_value)
    } else {
        None
    }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.

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

169-201: Align deletion order with comment and reduce race potential

Comment says “disk, scheduled tasks, then memory” but code does disk → memory → scheduled. Deleting the task before removing from memory can also prevent a transient window where a scheduled task runs while memory entry is already gone.

     // delete from Object Store
     store
         .delete_object(&alert_path)
         .await
         .map_err(AlertError::ObjectStorage)?;
 
-    // delete from memory
-    alerts.delete(alert_id).await?;
-
-    // delete the scheduled task
-    alerts.delete_task(alert_id).await?;
+    // delete the scheduled task
+    alerts.delete_task(alert_id).await?;
+
+    // delete from memory
+    alerts.delete(alert_id).await?;

src/alerts/mod.rs (2)

593-603: Honor the alert’s current notification_state in context

Using NotificationState::Notify here ignores the configured mute/snooze state and can lead to notifications being sent when they shouldn’t. Pass through self.notification_state.

-            AlertInfo::new(
+            AlertInfo::new(
                 self.id,
                 self.title.clone(),
                 self.state,
-                alert_enums::NotificationState::Notify,
+                self.notification_state.clone(),
                 self.severity.clone().to_string(),
             ),

---

886-911: Return the right HTTP status for authorization and enterprise-only features

• Unauthorized should map to 403 Forbidden (or 401 if you expect auth challenges), not 400.
• NotPresentInOSS could be 403 (entitlement) or 501; 403 is usually clearer.

-            Self::Unauthorized => StatusCode::BAD_REQUEST,
+            Self::Unauthorized => StatusCode::FORBIDDEN,
@@
-            Self::NotPresentInOSS(_) => StatusCode::BAD_REQUEST,
+            Self::NotPresentInOSS(_) => StatusCode::FORBIDDEN,

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

417-445: Authorization added to evaluate_alert — looks good

The earlier auth gap is now addressed by extracting the session and calling user_auth_for_query. Also consider switching to a read lock for ALERTS here.

-    let guard = ALERTS.write().await;
+    let guard = ALERTS.read().await;

src/alerts/mod.rs (1)

829-833: Compile error: sum() over Result<usize, _> doesn’t implement Sum

This repeats a previously raised issue. Use try_fold to accumulate and propagate errors.

-            plan.inputs()
-                .iter()
-                .map(|input| _get_number_of_agg_exprs(input))
-                .sum()
+            plan.inputs()
+                .iter()
+                .try_fold(0usize, |acc, input| {
+                    _get_number_of_agg_exprs(input).map(|n| acc + n)
+                })

src/handlers/http/alerts.rs (4)

120-126: Prefer read lock when only accessing the manager instance

You’re only reading ALERTS to fetch the manager; using a write lock increases contention unnecessarily.

-    let guard = ALERTS.write().await;
+    let guard = ALERTS.read().await;

---

279-285: Don’t pass empty string as trigger message for state updates

Passing Some("".into()) is ambiguous. If no notification needs to be triggered, pass None.

-    alerts
-        .update_state(alert_id, AlertState::Disabled, Some("".into()))
+    alerts
+        .update_state(alert_id, AlertState::Disabled, None)
         .await?;

---

317-323: Same here: use None for optional trigger message

-    alerts
-        .update_state(alert_id, AlertState::NotTriggered, Some("".into()))
+    alerts
+        .update_state(alert_id, AlertState::NotTriggered, None)
         .await?;

---

336-342: Use a read lock for fetching the manager

This handler only needs to read the ALERTS option and then call methods on the manager; a read lock is sufficient.

-    let guard = ALERTS.write().await;
+    let guard = ALERTS.read().await;

src/alerts/mod.rs (3)

742-760: Avoid string matching for aggregate detection where possible

extract_aggregate_aliases falls back to string contains for “count/sum/avg/min/max”. Prefer exhaustive pattern matching over Expr variants to avoid false positives across DataFusion versions.

If supporting all variants is complex right now, consider adding comments and a unit test to guard this behavior across DataFusion upgrades.

Also applies to: 763-808

---

1000-1005: Comment drift: “paused” vs “disabled”

The comment still mentions “paused” while the state is Disabled. Update the comment to avoid confusion.

-    // Create alert task iff alert's state is not paused
+    // Create alert task iff alert is not disabled

---

1033-1092: Nice concurrency: release lock before auth checks; minor clone optimization

Releasing the map lock before auth checks is correct. In the tags-empty branch you clone session for each future. You can move one clone outside the iterator to avoid repeated clones.

-            let futures: Vec<_> = all_alerts
+            let session = session.clone();
+            let futures: Vec<_> = all_alerts
                 .into_iter()
                 .map(|alert| async {
-                    if user_auth_for_query(&session.clone(), &alert.query)
+                    if user_auth_for_query(&session, &alert.query)
                         .await
                         .is_ok()
                     {
                         Some(alert)
                     } else {
                         None
                     }
                 })
                 .collect();

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

src/migration/metadata_migration.rs (2)

114-118: Eliminate double-unwrap on server_mode and accept legacy capitalization.

Current code can panic if server_mode exists but is not a string. Also, handle both "All" and "all" to be robust to historical casing.

Apply this diff:

-    let sm = metadata.get("server_mode");
- 
-    if sm.is_none() || sm.unwrap().as_str().unwrap() == "All" {
+    // Guard against non-string values and handle legacy capitalization
+    if matches!(
+        metadata.get("server_mode").and_then(|v| v.as_str()),
+        None | Some("All") | Some("all")
+    ) {
         metadata.insert("server_mode".to_string(), json!(PARSEABLE.options.mode));
     }

---

62-108: Add unit tests for metadata migration functions

We currently lack tests for src/migration/metadata_migration.rs—all coverage today is in stream migrations. Please add targeted, table-driven unit tests for:

• v2_v3
  • Version bumps to “v3”
  • “ingester” → “ingestor” normalization in each user’s role array
  • Resulting roles field populated correctly

• v3_v4
  • Version bumps to “v4”
  • server_mode inserted from PARSEABLE.options.mode
  • Handle cases where the field is missing or set to "All"/"all"

• v4_v5
  • Removal of version field
  • Role/value behavior for "Query"/"Querier"/"all"/"All" once variants are finalized

• v5_v6
  • Removal of resource.tag
  • Backfill of user_groups

Suggested test locations:

• src/migration/metadata_migration.rs, around functions v2_v3…v5_v6

If it’s helpful, I can scaffold table-driven Rust tests for each path.

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

361-365: Apply consistent ceiling division logic as in POST handler

Maintain consistency with the POST handler by using the same ceiling division logic for calculating notification retries.

Apply this diff to match the POST handler logic:

-    let times = if (eval_freq / notif_freq) == 0 {
-        1
-    } else {
-        (eval_freq / notif_freq) as usize
-    };
+    let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
+    let times = usize::try_from(times_u64).unwrap_or(usize::MAX);

Consider extracting this into a helper function to avoid duplication:

fn calculate_notification_retries(eval_freq: u64, notif_freq: u64) -> usize {
    let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
    usize::try_from(times_u64).unwrap_or(usize::MAX)
}

---

220-223: Fix: Cannot add std::time::Duration directly to DateTime<Utc>

The code attempts to add a std::time::Duration (from humantime::parse_duration) directly to DateTime<Utc>, which doesn't compile. You need to convert it to chrono::Duration first.

Apply this diff to fix the compilation error:

-            let till_time = if let Ok(duration) =
-                humantime::parse_duration(&new_notification_state.state)
-            {
-                (Utc::now() + duration).to_rfc3339()
+            let till_time = if let Ok(duration) =
+                humantime::parse_duration(&new_notification_state.state)
+            {
+                let chrono_dur = chrono::Duration::from_std(duration)
+                    .map_err(|_| AlertError::InvalidStateChange("Invalid duration overflow".into()))?;
+                (Utc::now() + chrono_dur).to_rfc3339()

---

98-102: Use ceiling division for consistent notification retry calculations

The current floor division logic can result in times = 0 when notif_freq > eval_freq, which would then be set to 1 but creates semantic inconsistency. Using ceiling division better captures the "number of notifications within one eval window" intent.

Apply this diff to use ceiling division:

-    let times = if (eval_freq / notif_freq) == 0 {
-        1
-    } else {
-        (eval_freq / notif_freq) as usize
-    };
+    let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
+    let times = usize::try_from(times_u64).unwrap_or(usize::MAX);

src/alerts/mod.rs (2)

561-566: Handle v1 "silenced" state migration properly

The v1 state "silenced" should map to v2 AlertState::Disabled for backward compatibility, but this case is missing. Without it, silenced alerts will incorrectly be migrated as NotTriggered.

Apply this diff to handle the "silenced" state:

     fn extract_state(alert_json: &JsonValue) -> AlertState {
         let state_str = alert_json["state"].as_str().unwrap_or("resolved");
         match state_str.to_lowercase().as_str() {
             "triggered" => AlertState::Triggered,
             "resolved" => AlertState::NotTriggered,
+            "silenced" => AlertState::Disabled,
             _ => AlertState::NotTriggered,
         }
     }

---

829-832: Fix: Iterator::sum() over Result values does not compile

The code attempts to call sum() on an iterator of Result<usize, _> values, but Result doesn't implement the Sum trait. This will fail to compile.

Apply this diff using try_fold to properly handle the Result types:

-            plan.inputs()
-                .iter()
-                .map(|input| _get_number_of_agg_exprs(input))
-                .sum()
+            plan.inputs()
+                .iter()
+                .try_fold(0usize, |acc, input| {
+                    _get_number_of_agg_exprs(input).map(|n| acc + n)
+                })

src/migration/metadata_migration.rs (2)

80-80: Typo in panic message.

"arrray" → "array".

-            panic!("privileges is an arrray")
+            panic!("privileges is an array")

---

88-92: DRY the privilege normalization into a helper.

The "ingester" → "ingestor" normalization appears in two places. Extracting it avoids duplication and future drift.

Example helper and usage (apply outside the selected lines as appropriate):

fn normalize_privilege_value(value: &mut JsonValue) {
    if matches!(value.as_str(), Some("ingester")) {
        *value = JsonValue::String("ingestor".to_string());
    }
}

Then replace the inlined checks with:

if let Some(value) = privilege.get_mut("privilege") {
    normalize_privilege_value(value);
}

Also applies to: 127-131

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

395-413: Persist to storage before updating in-memory state

The current order updates in-memory state before persisting to storage. If persistence fails, the in-memory state would be inconsistent with stored state. Consider persisting first, then updating memory.

Apply this diff to ensure consistency:

-    // save the new alert in ObjectStore
-    alerts.update(new_alert).await;
     let path = alert_json_path(*new_alert.get_id());
     let store = PARSEABLE.storage.get_object_store();
     let alert_bytes = serde_json::to_vec(&new_alert.to_alert_config())?;
     store.put_object(&path, Bytes::from(alert_bytes)).await?;

+    // update memory only after successful persistence
+    alerts.update(new_alert).await;
+
     // remove the task
     alerts.delete_task(alert_id).await?;

     // remove alert from memory
     alerts.delete(alert_id).await?;

     let config = new_alert.to_alert_config().to_response();

     // start the task
     alerts.start_task(new_alert.clone_box()).await?;

src/alerts/target.rs (1)

452-454: Fix typo in disabled state message

There's a typo in the disabled state message returned by default_disabled_string(). The message says "till the sate is disabled" but should say "state".

The typo is in the alert_structs.rs file as shown in the code snippets. Consider fixing it:

-            "{} is now `disabled`. No more evals will be run till the sate is `disabled`.",
+            "{} is now `disabled`. No more evals will be run till the state is changed.",

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.

src/handlers/http/alerts.rs (5)

406-433: Consider adding authorization checks to evaluate_alert

The evaluate_alert endpoint allows forcing immediate alert evaluation. While it does check user access to the alert's query, consider whether additional authorization is needed to prevent abuse of this functionality.

The current implementation validates query access, but you may want to add rate limiting or additional permission checks to prevent users from triggering excessive evaluations.

---

95-104: Use ceiling division for notification retry calculation

The current floor division can result in times = 0 when notif_freq > eval_freq, which gets coerced to 1. This doesn't accurately represent the retry behavior. Use ceiling division for more accurate retry count calculation.

Apply this fix to use ceiling division:

 // calculate the `times` for notification config
 let eval_freq = alert.get_eval_frequency();
 let notif_freq = alert.notification_config.interval;
-let times = if (eval_freq / notif_freq) == 0 {
-    1
-} else {
-    (eval_freq / notif_freq) as usize
-};
+// Use ceiling division: (a + b - 1) / b for b > 0
+let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
+let times = usize::try_from(times_u64).unwrap_or(usize::MAX);

---

220-224: Fix compilation error: Cannot add std::time::Duration to DateTime

humantime::parse_duration returns std::time::Duration which cannot be directly added to DateTime<Utc>. You need to convert it to chrono::Duration first.

Apply this fix to convert the duration properly:

 let till_time = if let Ok(duration) =
     humantime::parse_duration(&new_notification_state.state)
 {
-    (Utc::now() + duration).to_rfc3339()
+    let chrono_dur = chrono::Duration::from_std(duration)
+        .map_err(|_| AlertError::InvalidStateChange("Invalid duration overflow".into()))?;
+    (Utc::now() + chrono_dur).to_rfc3339()

---

358-365: Apply consistent ceiling division for notification retries in update path

The notification retry calculation here uses floor division, which differs from the recommended ceiling division approach in the POST handler. This inconsistency could lead to different behavior between creation and update.

Apply the same ceiling division logic:

 // Calculate notification config
 let eval_freq = new_config.get_eval_frequency();
 let notif_freq = new_config.notification_config.interval;
-let times = if (eval_freq / notif_freq) == 0 {
-    1
-} else {
-    (eval_freq / notif_freq) as usize
-};
+// Use ceiling division for consistency
+let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
+let times = usize::try_from(times_u64).unwrap_or(usize::MAX);

Consider extracting this calculation into a helper function to avoid duplication between POST and PUT handlers:

fn calculate_notification_retries(eval_freq: u64, notif_freq: u64) -> usize {
    let times_u64 = std::cmp::max(1, (eval_freq + notif_freq - 1) / notif_freq);
    usize::try_from(times_u64).unwrap_or(usize::MAX)
}

---

389-402: Persist to storage before updating in-memory state

The code updates the in-memory state before persisting to storage. If persistence fails, the system will have inconsistent state between memory and disk. This could lead to data loss on restart.

Reorder operations to persist first:

-// Perform I/O operations
 let path = alert_json_path(*new_alert.get_id());
 let store = PARSEABLE.storage.get_object_store();
 let alert_bytes = serde_json::to_vec(&new_alert.to_alert_config())?;
 store.put_object(&path, Bytes::from(alert_bytes)).await?;

-// Now perform the atomic operations
+// Only update in-memory state after successful persistence
 alerts.delete_task(alert_id).await?;
 alerts.delete(alert_id).await?;
 alerts.update(&*new_alert).await;
 alerts.start_task(new_alert.clone_box()).await?;

src/alerts/mod.rs (2)

564-566: Add support for v1 "silenced" state during migration

The v1 "silenced" state is commented out in the migration logic. This will cause silenced v1 alerts to be incorrectly migrated as NotTriggered instead of Disabled.

Uncomment the "silenced" case to properly handle v1 alerts:

 match state_str.to_lowercase().as_str() {
     "triggered" => AlertState::Triggered,
     "resolved" => AlertState::NotTriggered,
+    "silenced" => AlertState::Disabled,
     _ => AlertState::NotTriggered,
 }

---

826-832: Iterator::sum() over Result values does not compile

The code attempts to use sum() on an iterator of Result<usize, _> values, but Result doesn't implement the Sum trait. This will cause a compilation error.

Replace with try_fold to properly handle Results:

 // Use inputs() method to get all input plans
-plan.inputs()
-    .iter()
-    .map(|input| _get_number_of_agg_exprs(input))
-    .sum()
+plan.inputs()
+    .iter()
+    .try_fold(0usize, |acc, input| {
+        _get_number_of_agg_exprs(input).map(|n| acc + n)
+    })

src/alerts/target.rs (1)

271-299: Potential deadlock risk when accessing ALERTS within spawned task

The spawned timeout task acquires a read lock on ALERTS to fetch alert state. If this runs while other operations hold write locks on ALERTS, it could contribute to lock contention. Consider passing necessary state data to avoid repeated global lock access.

Consider refactoring to minimize lock access within the spawned task by passing the necessary data upfront or using a more granular locking strategy.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.