[Detail Bug] Object store stats: concurrent observations can make exponential-decay error rates non-deterministic

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_89c93691-d025-417f-ade7-84261c5a6c1e

Introduced in 2c1010e by @shikhar on Sep 5, 2025

Summary

• Context: Instant::now() captured outside the lock in observe() violates the exponential decay model's mathematical invariant, producing non-deterministic error rates under concurrent access.
• Bug: observe() captures now before acquiring the per-bucket lock, so two concurrent calls can apply timestamps out of order and write last_update backwards.
• Actual vs. expected: Actual: When now < last_update, duration_since() saturates to zero so no decay is applied, producing different final error rates/score for the same set of observations depending on lock acquisition order. Expected: The decay model should be deterministic for a given sequence of observations; last_update should be monotonically increasing so decay is applied consistently.
• Impact: Non-deterministic and mathematically incorrect error-rate/score calculations for a bucket under concurrent observe() calls; can corrupt scores by a few points per backwards-timestamp event and accumulate with repeats.

Code with Bug

pub fn observe(&self, bucket: BucketName, outcome: Result<Duration, ()>) {
    let now = Instant::now();  // <-- BUG 🔴 captured before lock; can be older than last_update
    let entry = self.by_bucket.entry(bucket).or_default();
    let mut stats = entry.lock();
    // ...
    stats.last_update = now;  // <-- BUG 🔴 last_update can move backwards under concurrency
}

fn error_rate(&self, now: Instant) -> f64 {
    let elapsed = now.duration_since(self.last_update).as_secs_f64();
    self.error_rate * (-ALPHA * elapsed).exp() // <-- BUG 🔴 if now < last_update, elapsed becomes 0 and decay is skipped
}

Explanation

The exponential decay computation assumes time moves forward: elapsed is derived from now.duration_since(last_update). If two requests for the same bucket call observe() concurrently, they can capture now at different times but acquire the lock in the opposite order. The second locker may then write an older timestamp to last_update. When later computing decay (or when applying decay during subsequent observations), now < last_update causes duration_since() to saturate to zero, skipping decay.

This violates the model invariant that the final error rate should be deterministic given the same observations: applying the same two observations with timestamps t=50ms and t=100ms yields different outputs depending on which one updates state first (because one ordering produces a backwards timestamp and a “no decay” step).

Recommended Fix

Move Instant::now() inside the per-bucket lock so last_update reflects “time of state update” in lock-serialized order:

pub fn observe(&self, bucket: BucketName, outcome: Result<Duration, ()>) {
    let entry = self.by_bucket.entry(bucket).or_default();
    let mut stats = entry.lock();
    let now = Instant::now();
    // ... rest unchanged ...
}

History

This bug was introduced in commit 2c1010e. The initial commit created the entire codebase from scratch (26 files, 10,207 lines), and the observe() function captured Instant::now() outside the lock from the very beginning. The bug slipped in because the original implementation prioritized getting the code working without considering the concurrent access implications on the decay model's time ordering requirements.