fix(reprocessing): Remove strict equality from check

[tobias-wilfert]

ref: INC-2491

[linear-code]

INC-2491

[tobias-wilfert]

@cursoragent review

[cursor]

Now let me create a comprehensive review of this change.

PR Review: Remove strict equality from reprocessing check

Summary

This PR fixes a race condition in the reprocessing counter logic where concurrent decrements could cause the counter to go negative, preventing finish_reprocessing from ever being triggered.

The Change

File: src/sentry/services/eventstore/reprocessing/redis.py

Changed the completion check from == 0 to <= 0 to handle cases where the counter overshoots due to concurrent decrements.

Analysis

✅ The Fix is Correct

The change addresses a real concurrency bug:

1. Root cause: Redis DECRBY is atomic per operation, but multiple concurrent calls to mark_event_reprocessed() can race:

   • Thread A: decrements counter from 2 to 0
   • Thread B: decrements counter from 0 to -2
   • Original code: both return False (checking == 0), finish_reprocessing never called
   • Fixed code: both return True (checking <= 0), finish_reprocessing properly triggered

2. Why the original bug was severe: If the counter went negative, reprocessing would hang forever since no task would trigger completion.

3. Why the fix is safe: Even if multiple threads now trigger finish_reprocessing.delay():

   • The Celery task uses Group.objects.select_for_update() for database-level locking (line 287 in finish_reprocessing)
   • First task succeeds and deletes the group
   • Subsequent tasks fail with Group.DoesNotExist (harmless)
   • Much better than hanging forever!

🟡 Minor Concerns

1. Test Coverage Gap

The existing test doesn't cover the negative counter scenario:

# tests/sentry/services/eventstore/processing/test_redis_cluster.py
result = store.mark_event_reprocessed(group_id=group_id, num_events=0)  # Decrements by 0

Recommendation: Add a test case that verifies behavior when num_events causes the counter to go negative:

@use_redis_cluster()
def test_mark_event_reprocessed_negative_counter() -> None:
    """Test that completion is triggered even when counter goes negative due to concurrent decrements."""
    group_id = 5
    store = RedisReprocessingStore()
    date_created = datetime.now()
    
    # Start with sync_count of 3
    store.start_reprocessing(
        group_id=group_id, date_created=date_created, sync_count=3, event_count=5
    )
    
    # Simulate concurrent decrements that overshoot
    result1 = store.mark_event_reprocessed(group_id=group_id, num_events=2)  # 3 -> 1
    assert result1 is False
    
    result2 = store.mark_event_reprocessed(group_id=group_id, num_events=3)  # 1 -> -2
    assert result2 is True  # Should trigger completion even though negative

2. Error Handling in finish_reprocessing

The finish_reprocessing task doesn't explicitly handle the case where it's called multiple times (leading to Group.DoesNotExist on subsequent calls). While this won't break functionality, it could create noise in error logs.

Optional improvement:

def finish_reprocessing(project_id: int, group_id: int) -> None:
    from sentry.models.activity import Activity
    from sentry.models.group import Group
    from sentry.models.groupredirect import GroupRedirect

    try:
        with transaction.atomic(router.db_for_write(Group)):
            group = Group.objects.select_for_update().get(id=group_id)
            # ... rest of the logic
    except Group.DoesNotExist:
        # Already processed by another task, this is expected
        logger.info(
            "finish_reprocessing called for already-deleted group",
            extra={"group_id": group_id, "project_id": project_id}
        )
        return

3. Comment Quality

The inline comment is good but could be slightly more specific about the race condition:

# Concurrent mark_event_reprocessed calls can cause the counter to overshoot zero
# (e.g., two threads decrementing from 2 -> 0 -> -2). Use `<=` to ensure completion
# is triggered even when the counter goes negative.
return new_decremented_value <= 0

Verdict: ✅ APPROVED with minor suggestions

Critical: None - the fix is correct and addresses a real production bug (INC-2491)

Nice-to-have:

• Add test coverage for the negative counter scenario
• Consider adding error handling in finish_reprocessing for cleaner logging
• Slightly improve the inline comment

The core logic is sound and this is a good fix for a concurrency bug. The potential for duplicate finish_reprocessing calls is acceptable since the task is effectively idempotent (second call fails harmlessly).

  

[markstory]

Looks like it will be less wrong than before.