Replays Self-Serve Bulk Delete System

[ShashankFC]

Test 5nn---nReplicated from ai-code-review-evaluation/sentry-greptile#5

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 19s —— View job

---

Code Review Complete

I've reviewed this PR for the Replays Self-Serve Bulk Delete System. This is a large PR with 100 files changed (+2312/-981 lines) across multiple areas of the codebase. Below are my findings organized by severity.

---

Critical Issues

1. Security: Unbounded Thread Pool in Replay Deletion

File: src/sentry/replays/usecases/delete.py:74

with cf.ThreadPoolExecutor(max_workers=100) as pool:
    pool.map(_delete_if_exists, _make_recording_filenames(project_id, row))

Issue: The thread pool creates up to 100 threads per replay deletion. For replays with many segments, this could exhaust system resources. If multiple deletions happen concurrently, this could create thousands of threads.

Recommendation: Limit max_workers to a more reasonable number (e.g., 10-20) and rely on batching multiple replays instead of parallel thread execution per replay.

Fix this →

2. Potential Data Loss: Silent Exception Handling

File: src/sentry/replays/usecases/delete.py:78-83

def _delete_if_exists(filename: str) -> None:
    """Delete the blob if it exists or silence the 404."""
    try:
        storage_kv.delete(filename)
    except NotFound:
        pass

Issue: This function silences ALL exceptions from storage_kv.delete(), not just NotFound. Other failures (permissions, network errors, etc.) will be silently ignored, potentially leaving orphaned data.

Recommendation: Be more specific with exception handling:

def _delete_if_exists(filename: str) -> None:
    try:
        storage_kv.delete(filename)
    except NotFound:
        pass  # File doesn't exist, which is fine
    except Exception as e:
        logger.warning("Failed to delete replay segment", extra={"filename": filename, "error": str(e)})
        sentry_sdk.capture_exception(e)

Fix this →

3. TODO in Production Code with Detector Logic

File: src/sentry/workflow_engine/processors/delayed_workflow.py:437

if (
    evaluation.logic_result and workflow_id is None
):  # TODO: detector trigger passes. do something like create issue
    pass

Issue: This TODO indicates incomplete detector trigger logic. When a detector condition passes but there's no workflow_id, nothing happens. This could be a silent failure case.

Recommendation: Either implement the detector trigger logic or add logging/metrics to track when this condition occurs.

Fix this →

---

High Priority Issues

4. Performance: N+1 Query Pattern in Delayed Workflow

File: src/sentry/workflow_engine/processors/delayed_workflow.py:168

Organization.objects.filter(project__id=project_id).get().id

Issue: This query is executed inside a retry loop within execute_query. This could be optimized by passing the organization_id directly rather than looking it up each time.

Recommendation: Pass organization_id as a parameter or cache it to avoid repeated queries.

5. XXX Comments Indicating Technical Debt

Multiple XXX comments in the codebase indicate areas needing attention:

File: src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:179

# XXX: I have to deserialize this request so it can be "automatically" reserialized by the
# paginate method. This is less than ideal.
return json.loads(make_seer_request(request_data).decode("utf-8"))

File: src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:185

# XXX: Request isn't streaming. Limitation of Seer authentication. Would be much faster if we
# could stream the request data since the GCS download will (likely) dominate latency.

These indicate performance and architecture concerns that should be tracked.

6. Missing Permission Checks in Replay Endpoints

File: src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:52-65

The endpoint checks for feature flags but doesn't appear to check user permissions beyond basic authentication. Ensure replay access permissions are properly validated.

---

Medium Priority Issues

7. Browser Reporting Validation Logic Issue

File: src/sentry/issues/endpoints/browser_reporting_collector.py:50-60

def validate_timestamp(self, value: int) -> int:
    """Validate that age is absent, but timestamp is present."""
    if self.initial_data.get("age"):
        raise serializers.ValidationError("If timestamp is present, age must be absent")
    return value

def validate_age(self, value: int) -> int:
    """Validate that age is present, but not timestamp."""
    if self.initial_data.get("timestamp"):
        raise serializers.ValidationError("If age is present, timestamp must be absent")
    return value

Issue: Both validators raise errors if the other field is present, which will result in duplicate error messages when both fields are provided (as shown in test line 168-175). This is confusing for API consumers.

Recommendation: Use a single validate() method instead to handle mutual exclusivity cleanly.

8. Inconsistent Error Handling in Replay Summarization

File: src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:104-123

The fetch_error_details function catches all exceptions but only logs them to Sentry without returning any indication of partial failure. Consider returning a status indicator or logging which specific errors failed to load.

9. Hardcoded Sample Rate

File: src/sentry/issues/endpoints/browser_reporting_collector.py:127

sample_rate=1.0,  # XXX: Remove this once we have a ballpark figure

This 100% sampling rate should be reduced once initial data collection is complete to avoid metric explosion.

---

Low Priority / Code Quality Issues

10. TODO Comments Throughout Codebase

Multiple TODO comments indicate future work:

• Line 192, 200 in commit_context.py: Exception handling improvements needed for GitLab
• Line 227 in commit_context.py: Abstract method implementation for all integrations

11. Deprecated Test File

File: tests/sentry/migrations/test_0917_convert_org_saved_searches_to_views.py (DELETED)

Good - this test file was properly deleted, indicating the migration is now stable.

12. Type Safety in EventInstance Validation

File: src/sentry/workflow_engine/processors/delayed_workflow.py:93-97

The validator for occurrence_id returns None for empty strings, but this could mask data quality issues. Consider raising a validation error instead for malformed data.

---

Positive Observations

1. Good Test Coverage: The browser reporting collector has comprehensive test coverage including edge cases (lines 106-176 in test file)
2. Proper Error Logging: Most error paths include structured logging with appropriate context
3. Retry Logic: The delayed workflow processor implements proper retry logic with exponential backoff (lines 160-164)
4. Feature Flags: New functionality is properly gated behind feature flags
5. Null Safety: The fix for max_segment_id being null (line 89-90 in delete.py) properly handles edge cases

---

Testing Recommendations

1. Load Testing: Test replay deletion with high segment counts to validate thread pool behavior
2. Concurrent Deletion: Test multiple concurrent replay deletions to ensure resource limits are respected
3. Failure Scenarios: Test storage deletion failures to ensure they're properly logged
4. Browser Reporting: Monitor the sample_rate=1.0 metric carefully in production

---

Summary

This PR implements a significant feature for replay deletion but has some critical issues around resource management and error handling that should be addressed before merging. The most important fixes are:

1. Reduce thread pool size to prevent resource exhaustion
2. Improve exception handling in storage deletion
3. Implement or document the detector trigger logic
4. Review and optimize the N+1 query pattern

The codebase shows good practices overall with feature flags, comprehensive testing, and proper logging, but the technical debt indicated by XXX/TODO comments should be tracked and addressed.

---