Skip to content

Conversation

@Loaki07
Copy link
Contributor

@Loaki07 Loaki07 commented Oct 17, 2025

PR Type

Bug fix


Description

  • Guard histogram interval must be positive

  • Prevent misaligned cache time adjustments

  • Skip alignment when interval invalid


Diagram Walkthrough

flowchart LR
  A["write_results cache alignment"] -- "is_aggregate && interval present" --> B["validate interval > 0"]
  B -- "true" --> C["align start/end to interval"]
  B -- "false" --> D["skip alignment/adjustment"]
Loading

File Walkthrough

Relevant files
Bug fix
mod.rs
Add positive-interval guards to histogram alignment           

src/service/search/cache/mod.rs

  • Add interval > 0 check before histogram alignment.
  • Guard end-time cache adjustment with positive interval.
  • Prevent microsecond conversion for zero/invalid intervals.
+5/-1     

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Handling

Validate that the comparison interval > 0 is correct for the type of histogram_interval (e.g., unsigned vs signed). If interval is unsigned, the check may be redundant; if signed, ensure no underflow occurs when later multiplying by 1_000_000.

if is_aggregate
    && let Some(interval) = res.histogram_interval
    && interval > 0
{
    let interval = interval * 1000 * 1000; // convert to microseconds
    // next interval of start_time
    if (accept_start_time % interval) != 0 {
        accept_start_time = accept_start_time - (accept_start_time % interval) + interval;
    }
    // previous interval of end_time
    if (accept_end_time % interval) != 0 {
        need_adjust_end_time = true;
        accept_end_time = accept_end_time - (accept_end_time % interval) - interval;
    }
}

// 2. get the data time range, check if need to remove records with discard_duration
let delay_ts = second_micros(get_config().limit.cache_delay_secs);
let mut accept_end_time =
    std::cmp::min(Utc::now().timestamp_micros() - delay_ts, accept_end_time);
let last_rec_ts = get_ts_value(ts_column, res.hits.last().unwrap());
let first_rec_ts = get_ts_value(ts_column, res.hits.first().unwrap());
let data_start_time = std::cmp::min(first_rec_ts, last_rec_ts);
let data_end_time = std::cmp::max(first_rec_ts, last_rec_ts);
if data_start_time < accept_start_time || data_end_time > accept_end_time {
    res.hits.retain(|hit| {
        if let Some(hit_ts) = hit.get(ts_column)
            && let Some(hit_ts_datetime) = convert_ts_value_to_datetime(hit_ts)
        {
            let item_ts = hit_ts_datetime.timestamp_micros();
            // only keep the records within the accept time range
            item_ts >= accept_start_time && item_ts <= accept_end_time
        } else {
            true
        }
    });
    res.total = res.hits.len();
    res.size = res.hits.len() as i64;
}

// 3. check if the hits is empty
if res.hits.is_empty() {
    log::info!("[trace_id {trace_id}] No hits found for caching, skipping caching");
    return;
}

// 4. check if the time range is less than discard_duration
if (accept_end_time - accept_start_time) < delay_ts {
    log::info!("[trace_id {trace_id}] Time range is too short for caching, skipping caching");
    return;
}

// 5. adjust the cache time range
if need_adjust_end_time
    && is_aggregate
    && let Some(interval) = res.histogram_interval
    && interval > 0
{
    accept_end_time += interval * 1000 * 1000;
}
Overflow Risk

Multiplying interval by 1_000_000 and adding to time values could overflow if interval is large. Consider using checked or saturating arithmetic and/or bounds validation on interval.

    let interval = interval * 1000 * 1000; // convert to microseconds
    // next interval of start_time
    if (accept_start_time % interval) != 0 {
        accept_start_time = accept_start_time - (accept_start_time % interval) + interval;
    }
    // previous interval of end_time
    if (accept_end_time % interval) != 0 {
        need_adjust_end_time = true;
        accept_end_time = accept_end_time - (accept_end_time % interval) - interval;
    }
}

// 2. get the data time range, check if need to remove records with discard_duration
let delay_ts = second_micros(get_config().limit.cache_delay_secs);
let mut accept_end_time =
    std::cmp::min(Utc::now().timestamp_micros() - delay_ts, accept_end_time);
let last_rec_ts = get_ts_value(ts_column, res.hits.last().unwrap());
let first_rec_ts = get_ts_value(ts_column, res.hits.first().unwrap());
let data_start_time = std::cmp::min(first_rec_ts, last_rec_ts);
let data_end_time = std::cmp::max(first_rec_ts, last_rec_ts);
if data_start_time < accept_start_time || data_end_time > accept_end_time {
    res.hits.retain(|hit| {
        if let Some(hit_ts) = hit.get(ts_column)
            && let Some(hit_ts_datetime) = convert_ts_value_to_datetime(hit_ts)
        {
            let item_ts = hit_ts_datetime.timestamp_micros();
            // only keep the records within the accept time range
            item_ts >= accept_start_time && item_ts <= accept_end_time
        } else {
            true
        }
    });
    res.total = res.hits.len();
    res.size = res.hits.len() as i64;
}

// 3. check if the hits is empty
if res.hits.is_empty() {
    log::info!("[trace_id {trace_id}] No hits found for caching, skipping caching");
    return;
}

// 4. check if the time range is less than discard_duration
if (accept_end_time - accept_start_time) < delay_ts {
    log::info!("[trace_id {trace_id}] Time range is too short for caching, skipping caching");
    return;
}

// 5. adjust the cache time range
if need_adjust_end_time
    && is_aggregate
    && let Some(interval) = res.histogram_interval
    && interval > 0
{
    accept_end_time += interval * 1000 * 1000;
}

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent integer overflows

Guard against potential integer overflow when converting interval to microseconds
and when computing with accept_start_time. Use saturating_mul and checked_ (or
saturating_
) operations to avoid panic or wrap-around on large values. If overflow
would occur, skip alignment or log and return early.

src/service/search/cache/mod.rs [809-817]

 if is_aggregate
     && let Some(interval) = res.histogram_interval
     && interval > 0
 {
-    let interval = interval * 1000 * 1000; // convert to microseconds
-    // next interval of start_time
-    if (accept_start_time % interval) != 0 {
-        accept_start_time = accept_start_time - (accept_start_time % interval) + interval;
+    let interval_us = interval.saturating_mul(1_000_000); // convert to microseconds safely
+    if interval_us == 0 {
+        log::warn!("Histogram interval overflowed when converting to microseconds, skipping alignment");
+    } else {
+        // next interval of start_time
+        let rem = accept_start_time % interval_us;
+        if rem != 0 {
+            accept_start_time = accept_start_time
+                .saturating_sub(rem)
+                .saturating_add(interval_us);
+        }
+    }
Suggestion importance[1-10]: 7

__

Why: Suggestion correctly targets the new guard block and proposes safer arithmetic to prevent overflow when converting interval to microseconds and adjusting accept_start_time. It's a solid robustness improvement, though not a confirmed bug fix.

Medium
Saturate end time adjustment

Avoid overflow when converting interval to microseconds and adding to
accept_end_time. Use saturating_mul and saturating_add, and guard against zero after
conversion to prevent silent wrap-around. Log when adjustment is skipped due to
overflow.

src/service/search/cache/mod.rs [862-868]

 if need_adjust_end_time
     && is_aggregate
     && let Some(interval) = res.histogram_interval
     && interval > 0
 {
-    accept_end_time += interval * 1000 * 1000;
+    let interval_us = interval.saturating_mul(1_000_000);
+    if interval_us == 0 {
+        log::warn!("Histogram interval overflowed when converting to microseconds, skipping end_time adjustment");
+    } else {
+        accept_end_time = accept_end_time.saturating_add(interval_us);
+    }
 }
Suggestion importance[1-10]: 7

__

Why: This accurately addresses the added end-time adjustment block by using saturating math to avoid overflow. It improves safety with minimal behavior change, but impact is moderate rather than critical.

Medium

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Summary

Adds defensive checks to prevent division by zero when histogram_interval is 0 in the cache result writing logic.

The PR adds && interval > 0 checks at two locations in the write_results function:

  • Line 811: Before performing modulo operations to align cache start/end times with histogram intervals
  • Line 865: Before adjusting the end time based on the interval

While validate_and_adjust_histogram_interval in histogram_interval.rs should prevent 0 values by returning a default (line 151-156), this defensive check ensures safety if histogram_interval arrives from other code paths or edge cases.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is a defensive fix that prevents potential division by zero errors without altering any business logic. The checks are conservative and only skip processing when the interval would cause arithmetic errors.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/service/search/cache/mod.rs 5/5 Added defensive checks to prevent division by zero when histogram_interval is 0, protecting arithmetic operations in cache time range alignment

Sequence Diagram

sequenceDiagram
    participant Caller
    participant write_results
    participant Cache
    
    Caller->>write_results: Call with histogram_interval
    
    alt histogram_interval is Some(value)
        write_results->>write_results: Check interval > 0 (NEW)
        alt interval > 0
            write_results->>write_results: Convert to microseconds
            write_results->>write_results: Align accept_start_time
            write_results->>write_results: Align accept_end_time
            write_results->>write_results: Set need_adjust_end_time flag
        else interval <= 0
            write_results->>write_results: Skip alignment (prevented division by zero)
        end
    else histogram_interval is None
        write_results->>write_results: Skip alignment
    end
    
    write_results->>write_results: Remove records outside time range
    
    alt need_adjust_end_time is true
        write_results->>write_results: Check interval > 0 again (NEW)
        alt interval > 0
            write_results->>write_results: Adjust accept_end_time
        else interval <= 0
            write_results->>write_results: Skip adjustment
        end
    end
    
    write_results->>Cache: Cache filtered results to disk
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@testdino-playwright-reporter
Copy link

⚠️ Test Run Unstable


Author: Loaki07 | Branch: fix/write_results_hist_interval | Commit: fc2ac0b

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 364 342 0 19 3 94% 4m 38s

View Detailed Results

@testdino-playwright-reporter
Copy link

⚠️ Test Run Unstable


Author: Loaki07 | Branch: fix/write_results_hist_interval | Commit: fc2ac0b

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 364 342 0 19 3 94% 4m 41s

View Detailed Results

@testdino-playwright-reporter
Copy link

⚠️ Test Run Unstable


Author: Loaki07 | Branch: fix/write_results_hist_interval | Commit: fc2ac0b

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 364 343 0 19 2 94% 4m 39s

View Detailed Results

@uddhavdave uddhavdave merged commit 31afc7c into main Oct 17, 2025
32 checks passed
@uddhavdave uddhavdave deleted the fix/write_results_hist_interval branch October 17, 2025 09:21
uddhavdave pushed a commit that referenced this pull request Oct 27, 2025
### **PR Type**
Bug fix


___

### **Description**
- Guard histogram interval must be positive

- Prevent misaligned cache time adjustments

- Skip alignment when interval invalid


___

### Diagram Walkthrough


```mermaid
flowchart LR
  A["write_results cache alignment"] -- "is_aggregate && interval present" --> B["validate interval > 0"]
  B -- "true" --> C["align start/end to interval"]
  B -- "false" --> D["skip alignment/adjustment"]
```



<details> <summary><h3> File Walkthrough</h3></summary>

<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug
fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>mod.rs</strong><dd><code>Add positive-interval guards
to histogram alignment</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

src/service/search/cache/mod.rs

<ul><li>Add <code>interval > 0</code> check before histogram
alignment.<br> <li> Guard end-time cache adjustment with positive
interval.<br> <li> Prevent microsecond conversion for zero/invalid
intervals.</ul>


</details>


  </td>
<td><a
href="https://github.com/openobserve/openobserve/pull/8838/files#diff-f07bfacc0501b9c234e64b16c1dd8eb0ae8fcbe231f90c81fed72bcc94946f74">+5/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

</details>

___

---------

Signed-off-by: loaki07 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

☢️ Bug Something isn't working Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants