fix(datastore): use event ID instead of max(endtime) in replace_last_event

[Sinity]

Summary

Fixes #554 - a critical bug where heartbeat merges target the wrong event due to SQL query mismatch.

Problem

get_events uses ORDER BY starttime DESC LIMIT 1 while replace_last_event uses WHERE endtime = (SELECT max(endtime)). When events have the same starttime but different durations, these select different events.

This causes:

• Duplicate events accumulating
• Events with 0 duration (heartbeats never merge)
• Cache/DB inconsistency

Solution

• replace_last_event now takes event_id parameter and uses WHERE id = ?
• Check rows_affected and error if 0 rows updated (detects cache inconsistency)
• heartbeat passes last_event.id to ensure the correct event is updated

Testing

All existing tests pass including test_event_heartbeat.

Impact

In real-world data (8 months):

• 284,995 AFK events were actually ~10,816 after deduplication (96% duplicates)
• 92% of events had 0 duration (heartbeats never merged)

🤖 Generated with Claude Code

---

Important

Fixes critical bug in replace_last_event by using event_id for updates, ensuring consistency with get_events and preventing duplicate events.

• Behavior:
  • replace_last_event now uses event_id instead of max(endtime) to update events, ensuring consistency with get_events ordering.
  • Adds error handling in replace_last_event to detect cache inconsistencies if 0 rows are updated.
  • heartbeat function updated to pass last_event.id to replace_last_event.
• Testing:
  • All existing tests pass, including test_event_heartbeat.

^{This description was created by for e9ec01d. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to e9ec01d in 42 seconds. Click for details.

• Reviewed 51 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. aw-datastore/src/datastore.rs:551

• Draft comment:
  Changing the function signature to include an event_id parameter ensures that the update is targeted deterministically, which directly addresses the mismatch described in #554.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. aw-datastore/src/datastore.rs:560

• Draft comment:
  The SQL UPDATE now uses ‘WHERE bucketrow = ?1 AND id = ?5’, which correctly targets the specific event by its ID. This avoids ambiguity when multiple events share the same starttime.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. aw-datastore/src/datastore.rs:592

• Draft comment:
  Good job checking for zero rows affected. Returning an error when no rows are updated helps catch potential cache/DB inconsistencies immediately.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. aw-datastore/src/datastore.rs:639

• Draft comment:
  Extracting the event_id from last_event in the heartbeat function ensures that the correct event is updated. The error handling for a missing ID is appropriate.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_MYWpdUaHZvG1U4Vx

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[greptile-apps]

Greptile Overview

Greptile Summary

Fixed critical heartbeat merge bug by aligning event selection logic between get_events and replace_last_event.

Problem: get_events uses ORDER BY starttime DESC LIMIT 1 while replace_last_event used WHERE endtime = (SELECT max(endtime)). When events have the same starttime but different durations, these queries select different events, causing heartbeat merges to target the wrong event.

Changes:

• replace_last_event now takes an event_id parameter and uses WHERE id = ? for precise targeting
• Added validation that checks rows_affected and returns an error if 0 rows were updated (detects cache/DB inconsistency)
• heartbeat method extracts last_event.id and passes it to replace_last_event
• Added error handling for missing event IDs

Impact: This fix prevents massive event duplication. In real-world data (8 months), this bug caused 284,995 AFK events when there should have been ~10,816 (96% duplicates), with 92% of events having 0 duration because heartbeats never merged.

Confidence Score: 5/5

• This PR is safe to merge with high confidence
• The fix is well-targeted, addresses a clear root cause, and improves correctness. The change uses event IDs (which are unique and stable) instead of max(endtime) which could select different events. The addition of validation (checking rows_affected) adds defensive programming to detect inconsistencies early. All existing tests pass, and the logic is straightforward without edge cases.
• No files require special attention

Important Files Changed

Filename	Overview
aw-datastore/src/datastore.rs	Fixed critical bug where heartbeat merges targeted wrong event by using event ID instead of max(endtime), added validation to detect cache/DB inconsistency

Sequence Diagram

sequenceDiagram
    participant Client
    participant Datastore
    participant Cache
    participant DB
    participant Transform

    Client->>Datastore: heartbeat(bucket_id, heartbeat_event, pulsetime)
    Datastore->>Cache: check last_heartbeat cache
    
    alt Cache hit
        Cache-->>Datastore: return cached last_event
    else Cache miss
        Datastore->>DB: get_events(LIMIT 1, ORDER BY starttime DESC)
        DB-->>Datastore: return last_event with ID
    end
    
    Datastore->>Transform: heartbeat(last_event, heartbeat_event, pulsetime)
    
    alt Data matches and within pulsetime
        Transform-->>Datastore: return merged_heartbeat
        Note over Datastore: Extract event_id from last_event
        Datastore->>DB: UPDATE events WHERE id = event_id
        DB-->>Datastore: rows_affected
        
        alt rows_affected == 0
            Datastore-->>Client: Error: cache/DB inconsistency
        else rows_affected > 0
            Datastore->>Cache: update last_heartbeat cache
            Datastore-->>Client: return merged_heartbeat
        end
    else Data differs or outside pulsetime
        Transform-->>Datastore: return None
        Datastore->>DB: insert_events(new heartbeat)
        DB-->>Datastore: new event with ID
        Datastore->>Cache: update last_heartbeat cache
        Datastore-->>Client: return new event
    end

Loading

[greptile-apps]

_{No files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[Sinity]

It was @claude, btw. lol

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.82%. Comparing base (656f3c9) to head (e9ec01d).
⚠️ Report is 16 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-datastore/src/datastore.rs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
- Coverage   70.81%   67.82%   -3.00%     
==========================================
  Files          51       54       +3     
  Lines        2916     3139     +223     
==========================================
+ Hits         2065     2129      +64     
- Misses        851     1010     +159     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ErikBjare]

@Sinity Thanks a lot for linking the transcript, I enjoyed reading it

[ErikBjare]

Lets hope this finally gets rid of all the 0-duration duplicate events.

Thanks a lot!