From 6c014543d79974252249e99d9cc6e15cf40222a7 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 16 Jul 2026 16:05:28 -0700 Subject: [PATCH 1/9] feat(derived): non-live row lifecycle for GroupDerivedData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add build-and-promote workflow: create temporary non-live rows in the background, drain the full action log into them, and atomically promote to live — replacing the old live row without disrupting reads. Includes project-wide batch processing with time-bounded tasks that self-reschedule, retry caps, and stale row cleanup. --- migrations_lockfile.txt | 2 +- src/sentry/issues/derived/processing.py | 345 +++++++++++++++--- src/sentry/issues/derived/tasks.py | 246 ++++++++++++- src/sentry/issues/models/groupderiveddata.py | 40 +- .../1141_add_is_live_to_group_derived_data.py | 86 +++++ src/sentry/options/defaults.py | 10 + .../sentry/issues/derived/test_processing.py | 293 ++++++++++++++- tests/sentry/issues/test_action_log.py | 6 +- tests/sentry/tasks/test_merge.py | 4 +- 9 files changed, 954 insertions(+), 78 deletions(-) create mode 100644 src/sentry/migrations/1141_add_is_live_to_group_derived_data.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index 026db7990b72..3307f526f448 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -31,7 +31,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0027_add_seer_run_coding_agent_handoff -sentry: 1140_organizationcontributors_unique_provider_hostname +sentry: 1141_add_is_live_to_group_derived_data social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index 8c501a3e1f3e..116d7291ac46 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -1,16 +1,17 @@ import enum import logging import time -from datetime import datetime, timedelta +from datetime import UTC, datetime, timedelta from django.core.exceptions import ObjectDoesNotExist from django.db import IntegrityError, router, transaction from django.db.models import Q +from sentry import options from sentry.issues.derived.aggregators import AGGREGATORS from sentry.issues.derived.framework import Pipeline from sentry.issues.derived.store import GroupDerivedDataStore -from sentry.issues.derived.tasks import process_group_log_task +from sentry.issues.derived.tasks import process_group_log_task, rebuild_group_derived_data_task from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData from sentry.models.group import Group @@ -30,28 +31,38 @@ class ProcessingStrategy(enum.Enum): INLINE = "inline" # try to process all pending actions quickly; fall back to ASYNC -def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData: - """Get or create the GroupDerivedData row for a group. +def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | None: + """Get the live GroupDerivedData row for a group, optionally creating one. + + When ``issues.derived-data.create-on-demand`` is enabled, a live row is + created if none exists. When disabled, returns None — processing will be + a no-op until a backfill task creates and promotes a row. Raises Group.DoesNotExist if the group has been deleted. """ + # Fast path: read-only check avoids a write attempt when the row exists + # or when on-demand creation is disabled. try: - return GroupDerivedData.objects.get(group_id=group_id) + return GroupDerivedData.objects.get(group_id=group_id, is_live=True) except GroupDerivedData.DoesNotExist: pass - try: - derived, _created = GroupDerivedData.objects.get_or_create( - group_id=group_id, - defaults={ - "cursor_date": EPOCH, - "cursor_id": 0, - "data": {}, - "pipeline_hash": pipeline_hash, - }, - ) - except IntegrityError: + if not options.get("issues.derived-data.create-on-demand"): + return None + + if not Group.objects.filter(id=group_id).exists(): raise Group.DoesNotExist(f"Group {group_id} does not exist") + + derived, _created = GroupDerivedData.objects.get_or_create( + group_id=group_id, + is_live=True, + defaults={ + "cursor_date": EPOCH, + "cursor_id": 0, + "data": {}, + "pipeline_hash": pipeline_hash, + }, + ) return derived @@ -68,34 +79,31 @@ def _entries_after_cursor( ) -def _cursor_lte(cursor_date: datetime, cursor_id: int) -> Q: - return Q(cursor_date__lt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__lte=cursor_id) - - def _process_batch( p: Pipeline[GroupActionLogEntry], derived: GroupDerivedData, - group_id: int, batch_size: int, ) -> bool: """ Process up to `batch_size` entries for a group. Updates derived in place. Returns True if there are more entries to process. - Concurrency: multiple callers may process the same group simultaneously. + Concurrency: multiple callers may process the same row simultaneously. Safety relies on two properties: 1. The action log is append-only and the pipeline is deterministic, so any caller processing the same entries produces the same result. - 2. The UPDATE uses a cursor guard (_cursor_lte) that only succeeds if no - other caller has already advanced the cursor past our batch. If it - fails (updated == 0), a concurrent caller already wrote a superset - of our work, so we refresh and check if more remains. + 2. The UPDATE uses a cursor guard scoped to the specific row (by id) + and pipeline_hash that only succeeds if no other caller has already + advanced the cursor past our batch. If it fails (updated == 0), a + concurrent caller already wrote a superset of our work, so we + refresh and check if more remains. This is an optimistic concurrency scheme — no locks are held, and the last-writer-wins semantics are safe because all writers compute the same deterministic result for overlapping entry ranges. """ + group_id = derived.group_id entries = _entries_after_cursor(group_id, derived.cursor_date, derived.cursor_id, batch_size) if not entries: @@ -109,8 +117,8 @@ def _process_batch( state_update = GroupDerivedDataStore.build_update(p, result) updated = GroupDerivedData.objects.filter( - Q(group_id=group_id) - & _cursor_lte(last_date, last_id) + Q(id=derived.id) + & (Q(cursor_date__lt=last_date) | Q(cursor_date=last_date, cursor_id__lte=last_id)) & Q(pipeline_hash=derived.pipeline_hash) ).update(cursor_date=last_date, cursor_id=last_id, **state_update) @@ -153,33 +161,75 @@ def _process_batch( class GroupLogTimeout(Exception): - """Raised when process_group_log cannot finish within its timeout.""" + """Raised when processing cannot finish within its time budget.""" + + def __init__(self, group_id: int, derived_id: int | None = None) -> None: + self.group_id = group_id + self.derived_id = derived_id + super().__init__(group_id) + + +DEFAULT_TIME_LIMIT = timedelta(seconds=8) + + +def _drain_log( + derived: GroupDerivedData, + batch_size: int = DEFAULT_BATCH_SIZE, + pipeline: Pipeline[GroupActionLogEntry] | None = None, + time_limit: timedelta = DEFAULT_TIME_LIMIT, +) -> bool: + """Process pending log entries into *derived*, batching as needed. + + Returns True if all entries were processed, False if the time limit was + reached and more entries remain. The limit is checked between batches, + so a single slow batch can exceed it. + """ + deadline = time.monotonic() + time_limit.total_seconds() + p = pipeline or PIPELINE + while _process_batch(p, derived, batch_size): + if time.monotonic() >= deadline: + return False + return True + + +# --------------------------------------------------------------------------- +# Live-row processing (incremental, on event arrival) +# --------------------------------------------------------------------------- def process_group_log( group_id: int, batch_size: int = DEFAULT_BATCH_SIZE, - target_pipeline: Pipeline[GroupActionLogEntry] | None = None, + pipeline: Pipeline[GroupActionLogEntry] | None = None, timeout: timedelta | None = None, -) -> GroupDerivedData: - """Fully drain all pending entries for a group, processing in batches. +) -> GroupDerivedData | None: + """Fully drain all pending entries for a group's live row. + Returns None if no live row exists and on-demand creation is disabled. Raises Group.DoesNotExist if the group has been deleted. Raises GroupLogTimeout if *timeout* elapses before all entries are processed. """ - p = target_pipeline or PIPELINE - timeout_seconds = timeout.total_seconds() if timeout is not None else None - start = time.monotonic() + p = pipeline or PIPELINE with transaction.atomic(using=router.db_for_write(GroupDerivedData)): derived = _ensure_derived(group_id, p.pipeline_hash) - has_more = _process_batch(p, derived, group_id, batch_size) - while has_more: - if timeout_seconds is not None and time.monotonic() - start >= timeout_seconds: - raise GroupLogTimeout(group_id) - has_more = _process_batch(p, derived, group_id, batch_size) + if derived is None: + return None + + if timeout is not None: + timeout_seconds = timeout.total_seconds() + start = time.monotonic() + has_more = _process_batch(p, derived, batch_size) + while has_more: + if time.monotonic() - start >= timeout_seconds: + raise GroupLogTimeout(group_id) + has_more = _process_batch(p, derived, batch_size) + else: + drained = _drain_log(derived, batch_size, p) + if not drained: + process_group_log_task.delay(group_id) return derived @@ -187,7 +237,7 @@ def process_group_log( def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) -> None: """Trigger derived data processing for a group. - Silently returns if the group has been deleted. + Silently returns if the group has been deleted or no live row exists. Strategy controls how processing is dispatched: SYNC — process all pending actions now @@ -216,7 +266,10 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) except ObjectDoesNotExist: return - has_more = _process_batch(pipeline, derived, group_id, INLINE_BATCH_SIZE) + if derived is None: + return + + has_more = _process_batch(pipeline, derived, INLINE_BATCH_SIZE) if has_more: # Derived data will be stale for any code running between now and # when the task completes. @@ -224,27 +277,217 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) process_group_log_task.delay(group_id) +# --------------------------------------------------------------------------- +# Non-live row lifecycle: create, build, promote, cleanup +# --------------------------------------------------------------------------- + + +def create_processing_row(group_id: int) -> GroupDerivedData: + """Create a new non-live GroupDerivedData row for background processing.""" + return GroupDerivedData.objects.create( + group_id=group_id, + is_live=False, + cursor_date=EPOCH, + cursor_id=0, + data={}, + ) + + +class PromotionResult(enum.Enum): + PROMOTED = "promoted" + CURSOR_BEHIND = "cursor_behind" + SUPERSEDED = "superseded" + CANDIDATE_MISSING = "candidate_missing" + RACE_LOST = "race_lost" + + +class _PromotionAborted(Exception): + pass + + +def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: + """Atomically promote a non-live row to live, replacing any existing live row. + + On success the old live row is deleted within the same transaction. + On any failure the transaction rolls back, restoring the old live row. + """ + try: + with transaction.atomic(using=router.db_for_write(GroupDerivedData)): + current_live = GroupDerivedData.objects.filter( + group_id=candidate.group_id, is_live=True + ).first() + + if current_live is not None: + # A candidate older than the live row means a concurrent or + # earlier build is trying to replace a newer one — reject it. + if candidate.id < current_live.id: + return PromotionResult.SUPERSEDED + if (candidate.cursor_date, candidate.cursor_id) < ( + current_live.cursor_date, + current_live.cursor_id, + ): + return PromotionResult.CURSOR_BEHIND + current_live.delete() + + updated = GroupDerivedData.objects.filter(id=candidate.id).update(is_live=True) + if not updated: + raise _PromotionAborted + + candidate.is_live = True + except _PromotionAborted: + return PromotionResult.CANDIDATE_MISSING + except IntegrityError: + return PromotionResult.RACE_LOST + + return PromotionResult.PROMOTED + + +MAX_PROMOTION_ATTEMPTS = 5 + + +def _get_or_create_processing_row(group_id: int, derived_id: int | None) -> GroupDerivedData | None: + """Resume an existing non-live row by id, or create a new one. + + Returns None if creation fails (IntegrityError) or the requested row + no longer exists (cleaned up). + """ + if derived_id is not None: + return GroupDerivedData.objects.filter(id=derived_id, is_live=False).first() + try: + return create_processing_row(group_id) + except IntegrityError: + return None + + +def build_and_promote_derived_data( + group_id: int, + batch_size: int = DEFAULT_BATCH_SIZE, + derived_id: int | None = None, + time_limit: timedelta = DEFAULT_TIME_LIMIT, +) -> None: + """Create (or resume) a non-live row, drain the log into it, and promote. + + When *derived_id* is provided, an existing non-live row is resumed instead + of creating a new one. This allows the task to be re-enqueued and pick up + where it left off after a time-limited drain. + + If promotion fails because the live row's cursor is ahead (it received + incremental updates while we were building), we drain additional entries + to catch up and retry. This avoids discarding a rebuild that contains + corrected historical data just because the live row is more current. + + Retries are bounded to avoid starvation if the live row is being updated + faster than we can catch up. On exhaustion the caller should re-enqueue. + + Raises GroupLogTimeout (with ``derived_id`` set) if the time-limited drain + could not finish, so the caller can decide its own retry strategy. + """ + derived = _get_or_create_processing_row(group_id, derived_id) + if derived is None: + logger.info( + "issues.derived.build_and_promote.no_row", + extra={"group_id": group_id, "derived_id": derived_id}, + ) + return + + result = PromotionResult.CURSOR_BEHIND + for attempt in range(MAX_PROMOTION_ATTEMPTS): + drained = _drain_log(derived, batch_size, time_limit=time_limit) + if not drained: + raise GroupLogTimeout(group_id, derived_id=derived.id) + result = promote_to_live(derived) + if result is PromotionResult.PROMOTED: + logger.info( + "issues.derived.promoted", + extra={ + "group_id": group_id, + "derived_id": derived.id, + "cursor_date": str(derived.cursor_date), + "cursor_id": derived.cursor_id, + "attempts": attempt + 1, + }, + ) + return + + if result is not PromotionResult.CURSOR_BEHIND: + break + + derived.delete() + + if result is PromotionResult.CURSOR_BEHIND: + metrics.incr("issues.derived.promotion_exhausted", sample_rate=1.0) + logger.warning( + "issues.derived.promotion_exhausted", + extra={ + "group_id": group_id, + "derived_id": derived.id, + "attempts": MAX_PROMOTION_ATTEMPTS, + }, + ) + + logger.info( + "issues.derived.promotion_rejected", + extra={ + "group_id": group_id, + "derived_id": derived.id, + "result": result.value, + }, + ) + + +def cleanup_stale_processing_rows( + max_age: timedelta = timedelta(days=2), +) -> int: + """Delete non-live rows older than *max_age* that were never promoted.""" + cutoff = datetime.now(UTC) - max_age + deleted, _ = GroupDerivedData.objects.filter( + is_live=False, + date_added__lt=cutoff, + ).delete() + return deleted + + +# --------------------------------------------------------------------------- +# Invalidation +# --------------------------------------------------------------------------- + + def invalidate_group_derived_data( group_id: int, cursor: tuple[datetime, int] | None = None, + *, + hard_delete: bool = True, ) -> None: - """Delete derived state so it is rebuilt from scratch on the next pass, - then kicks off an async task to regenerate the derived data. + """Invalidate derived state so it is rebuilt. + + *hard_delete* controls the strategy: + + - ``True`` (default): delete the live row immediately and kick off an + async task to rebuild from scratch. Use this when the existing data is + known to be wrong and must not be served. + - ``False``: leave the current live row in place and kick off a background + build-and-promote. The existing live row continues serving reads until + the replacement is ready. If *cursor* is ``(date_added, id)`` of the earliest affected entry, the - row is only deleted when its cursor is at or past that point; otherwise - the mutation is still ahead of processing and no invalidation is needed. - Without a cursor the invalidation is unconditional. + invalidation only fires when the live row's cursor is at or past that + point; otherwise the mutation is still ahead of processing and no + invalidation is needed. *cursor* is only meaningful with + ``hard_delete=True``. """ + if not hard_delete: + rebuild_group_derived_data_task.delay(group_id) + return + if cursor is None: - GroupDerivedData.objects.filter(group_id=group_id).delete() - process_group_log_task.delay(group_id) + GroupDerivedData.objects.filter(group_id=group_id, is_live=True).delete() + rebuild_group_derived_data_task.delay(group_id) return # Only invalidate if the row has already processed past the affected point. cursor_date, cursor_id = cursor deleted, _ = GroupDerivedData.objects.filter( - Q(group_id=group_id) + Q(group_id=group_id, is_live=True) & (Q(cursor_date__gt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id)), ).delete() if deleted: @@ -256,4 +499,4 @@ def invalidate_group_derived_data( "cursor_id": cursor_id, }, ) - process_group_log_task.delay(group_id) + rebuild_group_derived_data_task.delay(group_id) diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 96600a29c26a..324ca83d7134 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -11,7 +11,15 @@ BATCH_PROCESSING_DEADLINE = timedelta(seconds=30) # taskworker hard kill timeout BATCH_RETRIGGER_TIMEOUT = timedelta(seconds=20) # self-reschedule before the hard kill + _BATCH_TASK_KEY = "process_project_derived_data_batch" +_REBUILD_BATCH_TASK_KEY = "rebuild_project_derived_data_batch" +_REBUILD_GROUP_TASK_KEY = "rebuild_group_derived_data" + +# Cap self-rescheduling rebuilds to avoid infinite loops on very large groups. +_MAX_REBUILD_RUNS = 20 +# Hard limit on group IDs loaded per project-level task to bound memory. +_MAX_PROJECT_GROUPS = 10_000 @instrumented_task( @@ -30,6 +38,57 @@ def process_group_log_task(group_id: int, **kwargs: object) -> None: logger.info("process_group_log_task.group_not_found", extra={"group_id": group_id}) +@instrumented_task( + name="sentry.issues.derived.tasks.rebuild_group_derived_data_task", + namespace=issues_tasks, + silo_mode=SiloMode.CELL, +) +def rebuild_group_derived_data_task( + group_id: int, + derived_id: int | None = None, + prior_runs: int = 0, + **kwargs: object, +) -> None: + """Build a new GroupDerivedData row from scratch and promote it to live.""" + from taskbroker_client.state import current_task + + from sentry.issues.derived.processing import GroupLogTimeout, build_and_promote_derived_data + from sentry.taskworker.selfchain_idempotency import already_spawned, mark_spawned + + task_state = current_task() + activation_id = task_state.id if task_state else None + if activation_id and already_spawned(_REBUILD_GROUP_TASK_KEY, activation_id): + logger.info( + "rebuild_group_derived_data_task.duplicate_skipped", + extra={"group_id": group_id, "activation_id": activation_id}, + ) + metrics.incr( + "taskworker.selfchain.duplicate_skipped", + tags={"task": _REBUILD_GROUP_TASK_KEY}, + ) + return + + try: + build_and_promote_derived_data(group_id, derived_id=derived_id) + except GroupLogTimeout as e: + if prior_runs + 1 >= _MAX_REBUILD_RUNS: + logger.error( + "rebuild_group_derived_data_task.max_runs_exceeded", + extra={ + "group_id": group_id, + "derived_id": e.derived_id, + "prior_runs": prior_runs + 1, + }, + ) + metrics.incr("issues.derived.rebuild_max_runs_exceeded", sample_rate=1.0) + return + rebuild_group_derived_data_task.delay( + group_id, derived_id=e.derived_id, prior_runs=prior_runs + 1 + ) + if activation_id: + mark_spawned(_REBUILD_GROUP_TASK_KEY, activation_id) + + @instrumented_task( name="sentry.issues.derived.tasks.process_project_derived_data", namespace=issues_tasks, @@ -50,11 +109,12 @@ def process_project_derived_data(project_id: int, **kwargs: object) -> None: batch_size = options.get("issues.derived.project-batch-size") max_tasks = options.get("issues.derived.project-max-tasks") + # TODO: support very large projects via paginated iteration group_ids = list( Group.objects.filter(project_id=project_id) - .exclude(Exists(GroupDerivedData.objects.filter(group_id=OuterRef("id")))) + .exclude(Exists(GroupDerivedData.objects.filter(group_id=OuterRef("id"), is_live=True))) .order_by("id") - .values_list("id", flat=True) + .values_list("id", flat=True)[:_MAX_PROJECT_GROUPS] ) if not group_ids: @@ -64,6 +124,15 @@ def process_project_derived_data(project_id: int, **kwargs: object) -> None: ) return + if len(group_ids) >= _MAX_PROJECT_GROUPS: + logger.error( + "process_project_derived_data.too_many_groups", + extra={ + "project_id": project_id, + "limit": _MAX_PROJECT_GROUPS, + }, + ) + starts = [group_ids[i] for i in range(0, len(group_ids), batch_size)] ends = starts[1:] + [group_ids[-1] + 1] ranges = list(zip(starts, ends)) @@ -203,3 +272,176 @@ def process_project_derived_data_batch( "elapsed": time.monotonic() - start, }, ) + + +# --------------------------------------------------------------------------- +# Project-level rebuild: build-and-promote without deleting existing live rows +# --------------------------------------------------------------------------- + + +@instrumented_task( + name="sentry.issues.derived.tasks.rebuild_project_derived_data", + namespace=issues_tasks, + silo_mode=SiloMode.CELL, +) +def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: + """Rebuild derived data for all groups in a project via build-and-promote. + + Unlike process_project_derived_data (which only targets groups missing a + GDD row), this task covers every group in the project — creating derived + data where missing and replacing existing live rows with freshly built ones. + Existing live rows continue serving reads until each replacement is promoted. + """ + from sentry import options + from sentry.models.group import Group + + batch_size = options.get("issues.derived.project-batch-size") + max_tasks = options.get("issues.derived.project-max-tasks") + + # TODO: support very large projects via paginated iteration + group_ids = list( + Group.objects.filter(project_id=project_id) + .order_by("id") + .values_list("id", flat=True)[:_MAX_PROJECT_GROUPS] + ) + + if not group_ids: + return + + if len(group_ids) >= _MAX_PROJECT_GROUPS: + logger.error( + "rebuild_project_derived_data.too_many_groups", + extra={ + "project_id": project_id, + "limit": _MAX_PROJECT_GROUPS, + }, + ) + + starts = [group_ids[i] for i in range(0, len(group_ids), batch_size)] + ends = starts[1:] + [group_ids[-1] + 1] + ranges = list(zip(starts, ends)) + + if len(ranges) > max_tasks: + logger.error( + "rebuild_project_derived_data.too_many_tasks", + extra={ + "project_id": project_id, + "task_count": len(ranges), + "max_tasks": max_tasks, + }, + ) + return + + for start, end in ranges: + rebuild_project_derived_data_batch.delay( + project_id=project_id, + group_id_start=start, + group_id_end=end, + ) + + logger.info( + "rebuild_project_derived_data.scheduled", + extra={ + "project_id": project_id, + "group_count": len(group_ids), + "task_count": len(ranges), + }, + ) + + +@instrumented_task( + name="sentry.issues.derived.tasks.rebuild_project_derived_data_batch", + namespace=issues_tasks, + silo_mode=SiloMode.CELL, + processing_deadline_duration=int(BATCH_PROCESSING_DEADLINE.total_seconds()), +) +def rebuild_project_derived_data_batch( + project_id: int, + group_id_start: int, + group_id_end: int, + **kwargs: object, +) -> None: + """Rebuild derived data for groups in [group_id_start, group_id_end). + + Calls build_and_promote_derived_data for each group. On per-group + GroupLogTimeout, re-enqueues the individual group for resumption and + continues the batch. Reschedules the remaining range on batch timeout. + """ + from taskbroker_client.state import current_task + + from sentry.issues.derived.processing import GroupLogTimeout, build_and_promote_derived_data + from sentry.models.group import Group + from sentry.taskworker.selfchain_idempotency import already_spawned, mark_spawned + + task_state = current_task() + activation_id = task_state.id if task_state else None + if activation_id and already_spawned(_REBUILD_BATCH_TASK_KEY, activation_id): + logger.info( + "rebuild_project_derived_data_batch.duplicate_skipped", + extra={"project_id": project_id, "activation_id": activation_id}, + ) + metrics.incr( + "taskworker.selfchain.duplicate_skipped", + tags={"task": _REBUILD_BATCH_TASK_KEY}, + ) + return + + timeout_seconds = BATCH_RETRIGGER_TIMEOUT.total_seconds() + start = time.monotonic() + + group_ids = list( + Group.objects.filter( + project_id=project_id, + id__gte=group_id_start, + id__lt=group_id_end, + ) + .order_by("id") + .values_list("id", flat=True) + ) + + processed = 0 + rescheduled = False + + for group_id in group_ids: + remaining = timedelta(seconds=timeout_seconds - (time.monotonic() - start)) + try: + build_and_promote_derived_data(group_id, time_limit=remaining) + except GroupLogTimeout as e: + # Re-enqueue the single group with its id so the + # partially-drained row is resumed, then continue the batch. + rebuild_group_derived_data_task.delay(group_id, derived_id=e.derived_id) + processed += 1 + + if time.monotonic() - start >= timeout_seconds: + rescheduled = True + metrics.incr( + "issues.derived.rebuild_batch_rescheduled", + sample_rate=1.0, + tags={"reason": "batch_timeout"}, + ) + rebuild_project_derived_data_batch.delay( + project_id=project_id, + group_id_start=group_id + 1, + group_id_end=group_id_end, + ) + if activation_id: + mark_spawned(_REBUILD_BATCH_TASK_KEY, activation_id) + break + + metrics.incr( + "issues.derived.rebuild_project_groups_processed", + amount=processed, + sample_rate=1.0, + ) + logger.info( + "rebuild_project_derived_data_batch.complete", + extra={ + "project_id": project_id, + "group_id_start": group_id_start, + "group_id_end": group_id_end, + "processed": processed, + "total": len(group_ids), + "rescheduled": rescheduled, + "elapsed": time.monotonic() - start, + }, + ) diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index 93e4415620f4..2e632e2bee55 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -23,12 +23,19 @@ class GroupDerivedData(DefaultFieldsModel): """ Materialized state derived from GroupActionLogEntry entries. - One row per group. The cursor tracks the last entry processed. + + Multiple rows may exist per group, but at most one may be ``is_live=True`` + at any time (enforced by a partial unique constraint). Only the live row is + considered canonical; non-live rows are transient build artifacts. + + See ``processing.py`` for the full lifecycle and promotion protocol. """ __relocation_scope__ = RelocationScope.Excluded - group = FlexibleForeignKey("sentry.Group", unique=True) + group = FlexibleForeignKey("sentry.Group") + is_live = models.BooleanField(db_default=False, default=False) + cursor_date = models.DateTimeField(default=EPOCH) cursor_id = BoundedBigIntegerField(default=0) @@ -39,7 +46,6 @@ class GroupDerivedData(DefaultFieldsModel): # Column-backed features — promoted from JSON for indexing/querying. - # This is here just for demonstration purposes. view_count = BoundedPositiveIntegerField(default=0) # Stores the current Progress value as a string. progress = models.CharField(max_length=32, null=True, default="identified") @@ -55,9 +61,31 @@ class GroupDerivedData(DefaultFieldsModel): class Meta: app_label = "sentry" db_table = "sentry_groupderiveddata" + constraints = [ + models.UniqueConstraint( + fields=["group"], + condition=models.Q(is_live=True), + name="uniq_live_gdd_per_group", + ), + ] indexes = [ - models.Index(fields=["progress", "group"]), - models.Index(fields=["last_progressed_at", "group"]), + # Only live rows participate in joins/filters on these columns. + models.Index( + fields=["progress", "group"], + condition=models.Q(is_live=True), + name="sentry_gdd_progress_live", + ), + models.Index( + fields=["last_progressed_at", "group"], + condition=models.Q(is_live=True), + name="sentry_gdd_lastprog_live", + ), + models.Index(fields=["group", "is_live"]), + models.Index( + fields=["date_added"], + condition=models.Q(is_live=False), + name="sentry_gdd_stale_cleanup", + ), ] - __repr__ = sane_repr("group_id", "cursor_date", "cursor_id") + __repr__ = sane_repr("group_id", "is_live", "cursor_date", "cursor_id") diff --git a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py new file mode 100644 index 000000000000..e66bb8946de6 --- /dev/null +++ b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py @@ -0,0 +1,86 @@ +# Generated by Django 5.2.14 on 2026-07-09 15:43 + +import django.db.models.deletion +import sentry.db.models.fields.foreignkey +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. + # This should only be used for operations where it's safe to run the migration after your + # code has deployed. So this should not be used for most operations that alter the schema + # of a table. + # Here are some things that make sense to mark as post deployment: + # - Large data migrations. Typically we want these to be run manually so that they can be + # monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # run this outside deployments so that we don't block them. Note that while adding an index + # is a schema change, it's completely safe to run the operation after the code has deployed. + # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment + + is_post_deployment = False + + dependencies = [ + ("sentry", "1140_organizationcontributors_unique_provider_hostname"), + ] + + operations = [ + migrations.RemoveIndex( + model_name="groupderiveddata", + name="sentry_grou_progres_f1627f_idx", + ), + migrations.RemoveIndex( + model_name="groupderiveddata", + name="sentry_grou_last_pr_8c8185_idx", + ), + migrations.AddField( + model_name="groupderiveddata", + name="is_live", + field=models.BooleanField(db_default=False, default=False), + ), + migrations.AlterField( + model_name="groupderiveddata", + name="group", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + on_delete=django.db.models.deletion.CASCADE, to="sentry.group" + ), + ), + migrations.AddIndex( + model_name="groupderiveddata", + index=models.Index( + condition=models.Q(("is_live", True)), + fields=["progress", "group"], + name="sentry_gdd_progress_live", + ), + ), + migrations.AddIndex( + model_name="groupderiveddata", + index=models.Index( + condition=models.Q(("is_live", True)), + fields=["last_progressed_at", "group"], + name="sentry_gdd_lastprog_live", + ), + ), + migrations.AddIndex( + model_name="groupderiveddata", + index=models.Index(fields=["group", "is_live"], name="sentry_grou_group_i_2bcdf6_idx"), + ), + migrations.AddIndex( + model_name="groupderiveddata", + index=models.Index( + condition=models.Q(("is_live", False)), + fields=["date_added"], + name="sentry_gdd_stale_cleanup", + ), + ), + migrations.AddConstraint( + model_name="groupderiveddata", + constraint=models.UniqueConstraint( + condition=models.Q(("is_live", True)), + fields=("group",), + name="uniq_live_gdd_per_group", + ), + ), + ] diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 308f49b7bd78..c6e2f1ee0e80 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -3942,6 +3942,16 @@ flags=FLAG_AUTOMATOR_MODIFIABLE, ) +# When True, derived data processing creates a live GroupDerivedData row +# on demand for groups that don't have one yet. When False, processing is +# a no-op until a backfill task explicitly creates and promotes a row. +register( + "issues.derived-data.create-on-demand", + type=Bool, + default=True, + flags=FLAG_AUTOMATOR_MODIFIABLE, +) + # Issues derived data — process_project_derived_data task # Number of groups per batch task when fanning out project-wide processing. register( diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index 82dc25f7835e..e1678dc09fd8 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta, timezone +from unittest.mock import patch import pytest from django.db import router, transaction @@ -44,9 +45,14 @@ from sentry.issues.derived.processing import ( PIPELINE, GroupLogTimeout, + PromotionResult, _entries_after_cursor, + build_and_promote_derived_data, + cleanup_stale_processing_rows, + create_processing_row, invalidate_group_derived_data, process_group_log, + promote_to_live, ) from sentry.issues.derived.store import GroupDerivedDataStore from sentry.issues.models.groupactionlogentry import GroupActionLogEntry @@ -99,8 +105,10 @@ def test_records_and_processes(self) -> None: assert entries[0].actor_id == user.id derived = process_group_log(group.id) + assert derived is not None assert derived.cursor_id == entries[-1].id assert isinstance(derived.data, dict) + assert derived.is_live def test_incremental_processing(self) -> None: group = self.create_group() @@ -108,10 +116,12 @@ def test_incremental_processing(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None first_cursor = derived.cursor_id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.cursor_id > first_cursor def test_noop_when_no_new_entries(self) -> None: @@ -120,9 +130,11 @@ def test_noop_when_no_new_entries(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None old_updated = derived.date_updated derived = process_group_log(group.id) + assert derived is not None assert derived.date_updated == old_updated def test_process_group_log_only_affects_target(self) -> None: @@ -133,7 +145,7 @@ def test_process_group_log_only_affects_target(self) -> None: _publish(group=group_a, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group_b, action=ViewAction(), actor=GroupActionActor.user(user.id)) - cursor_b = GroupDerivedData.objects.get(group_id=group_b.id).cursor_id + cursor_b = GroupDerivedData.objects.get(group_id=group_b.id, is_live=True).cursor_id GroupActionLogEntry.objects.create( group_id=group_a.id, @@ -146,7 +158,7 @@ def test_process_group_log_only_affects_target(self) -> None: ) process_group_log(group_a.id) - assert GroupDerivedData.objects.get(group_id=group_b.id).cursor_id == cursor_b + assert GroupDerivedData.objects.get(group_id=group_b.id, is_live=True).cursor_id == cursor_b def test_batched_processing(self) -> None: group = self.create_group() @@ -157,6 +169,7 @@ def test_batched_processing(self) -> None: # Process in batches of 2 — should take 3 batches (2+2+1) derived = process_group_log(group.id, batch_size=2) + assert derived is not None entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) assert derived.cursor_id == entries[-1].id @@ -222,6 +235,7 @@ def test_status_starts_open(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) + assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[STATUS] == IssueStatus.OPEN @@ -231,6 +245,7 @@ def test_resolve_closes(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.data["status"] == "closed" def test_unresolve_reopens(self) -> None: @@ -240,6 +255,7 @@ def test_unresolve_reopens(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.data["status"] == "open" def test_duplicate_resolve_ignored(self) -> None: @@ -249,6 +265,7 @@ def test_duplicate_resolve_ignored(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.data["status"] == "closed" def test_duplicate_unresolve_ignored(self) -> None: @@ -257,6 +274,7 @@ def test_duplicate_unresolve_ignored(self) -> None: _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[STATUS] == IssueStatus.OPEN @@ -268,35 +286,36 @@ def test_status_toggle(self) -> None: _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.data["status"] == "closed" # --- invalidation --- - def test_invalidate_deletes_row(self) -> None: + def test_invalidate_deletes_live_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) process_group_log(group.id) - assert GroupDerivedData.objects.filter(group_id=group.id).exists() + assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() invalidate_group_derived_data(group.id) - assert not GroupDerivedData.objects.filter(group_id=group.id).exists() + assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() def test_invalidate_with_cursor_deletes_if_past(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) + assert derived is not None - # Cursor at the processed entry — row should be deleted. invalidate_group_derived_data(group.id, cursor=(derived.cursor_date, derived.cursor_id)) - assert not GroupDerivedData.objects.filter(group_id=group.id).exists() + assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() def test_invalidate_with_cursor_noop_if_not_reached(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) + assert derived is not None old_cursor = derived.cursor_id - # Cursor beyond what we've processed — row should be untouched. future = derived.cursor_date.replace(year=derived.cursor_date.year + 1) invalidate_group_derived_data(group.id, cursor=(future, old_cursor + 1000)) derived.refresh_from_db() @@ -308,12 +327,30 @@ def test_invalidate_then_reprocess(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.view_count == 2 invalidate_group_derived_data(group.id) derived = process_group_log(group.id) + assert derived is not None assert derived.view_count == 2 # rebuilt from scratch + def test_invalidate_soft_schedules_rebuild(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + derived = process_group_log(group.id) + assert derived is not None + original_id = derived.id + + with patch("sentry.issues.derived.tasks.rebuild_group_derived_data_task") as mock_task: + invalidate_group_derived_data(group.id, hard_delete=False) + mock_task.delay.assert_called_once_with(group.id) + + # Live row is still in place + derived.refresh_from_db() + assert derived.id == original_id + assert derived.is_live + def test_resolved_in_pull_request_proposes_fix(self) -> None: group = self.create_group() user = self.user @@ -324,6 +361,7 @@ def test_resolved_in_pull_request_proposes_fix(self) -> None: actor=GroupActionActor.user(user.id), ) derived = process_group_log(group.id) + assert derived is not None # An open PR referencing the issue proposes a fix; the issue stays open. assert derived.data["status"] == "open" assert derived.progress == IssueProgressState.FIX_PROPOSED.value @@ -339,6 +377,7 @@ def test_pull_request_close_demotes_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) + assert derived is not None assert derived.progress == IssueProgressState.FIX_PROPOSED.value _publish( @@ -347,6 +386,7 @@ def test_pull_request_close_demotes_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) + assert derived is not None assert derived.progress == IssueProgressState.DIAGNOSED.value def test_pull_request_close_with_remaining_keeps_progress(self) -> None: @@ -364,6 +404,7 @@ def test_pull_request_close_with_remaining_keeps_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) + assert derived is not None assert derived.progress == IssueProgressState.FIX_PROPOSED.value def test_pull_request_close_invalidate_and_replay_matches(self) -> None: @@ -382,12 +423,14 @@ def test_pull_request_close_invalidate_and_replay_matches(self) -> None: actor=actor, ) first = process_group_log(group.id) + assert first is not None first_data = first.data.copy() first_progress = first.progress first_last_progressed_at = first.last_progressed_at invalidate_group_derived_data(group.id) second = process_group_log(group.id) + assert second is not None assert second.data == first_data assert second.progress == first_progress @@ -398,6 +441,7 @@ def test_pipeline_hash_set_on_create(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) + assert derived is not None assert derived.pipeline_hash == PIPELINE.pipeline_hash def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: @@ -405,6 +449,7 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) + assert derived is not None first_cursor = derived.cursor_id # Insert a log entry directly to avoid inline processing from _publish @@ -422,7 +467,7 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: # between our load and the UPDATE in _process_batch. GroupDerivedData.objects.filter(group_id=group.id).update(pipeline_hash="reset") - processing._process_batch(processing.PIPELINE, derived, group.id, 1) + processing._process_batch(processing.PIPELINE, derived, 1) # The UPDATE should not have matched because the DB hash changed derived.refresh_from_db() @@ -436,8 +481,223 @@ def test_invalidate_and_reprocess_restores_pipeline_hash(self) -> None: invalidate_group_derived_data(group.id) derived = process_group_log(group.id) + assert derived is not None assert derived.pipeline_hash == PIPELINE.pipeline_hash + def test_noop_when_on_demand_disabled(self) -> None: + group = self.create_group() + + with self.options({"issues.derived-data.create-on-demand": False}): + # Publish with async strategy so inline processing doesn't create a row + with patch("sentry.issues.derived.tasks.process_group_log_task"): + _publish( + group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id) + ) + + derived = process_group_log(group.id) + + assert derived is None + assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + + +# --- Non-live row lifecycle --- + + +@with_feature("projects:issue-action-log-write-to-db") +class PromoteToLiveTest(TestCase): + def test_promote_with_no_existing_live(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + row = create_processing_row(group.id) + processing._drain_log(row) + assert promote_to_live(row) is PromotionResult.PROMOTED + assert row.is_live + + row.refresh_from_db() + assert row.is_live + assert row.view_count == 1 + + def test_promote_replaces_older_live(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + old = process_group_log(group.id) + assert old is not None + old_id = old.id + + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + new = create_processing_row(group.id) + processing._drain_log(new) + assert promote_to_live(new) is PromotionResult.PROMOTED + + assert not GroupDerivedData.objects.filter(id=old_id).exists() + assert new.is_live + assert new.view_count == 2 + + def test_promote_rejected_if_candidate_older(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + old_candidate = create_processing_row(group.id) + processing._drain_log(old_candidate) + + # Create and promote a newer candidate first + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + new_candidate = create_processing_row(group.id) + processing._drain_log(new_candidate) + assert promote_to_live(new_candidate) is PromotionResult.PROMOTED + + assert promote_to_live(old_candidate) is PromotionResult.SUPERSEDED + + # New candidate is still live + live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert live.id == new_candidate.id + + def test_promote_rejected_if_cursor_behind(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + # Create a live row that has processed everything + live = process_group_log(group.id) + assert live is not None + + # Create a candidate that only processes the first entry + candidate = create_processing_row(group.id) + processing._process_batch(PIPELINE, candidate, batch_size=1) + + # Candidate cursor is behind the live row + assert (candidate.cursor_date, candidate.cursor_id) < ( + live.cursor_date, + live.cursor_id, + ) + assert promote_to_live(candidate) is PromotionResult.CURSOR_BEHIND + + def test_build_and_promote(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(self.user.id)) + + build_and_promote_derived_data(group.id) + derived = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert derived.view_count == 1 + assert derived.data["status"] == "closed" + + def test_build_and_promote_replaces_stale_live(self) -> None: + group = self.create_group() + user = self.user + + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) + old = process_group_log(group.id) + assert old is not None + old_id = old.id + + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) + build_and_promote_derived_data(group.id) + new = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert new.id != old_id + assert new.view_count == 2 + assert not GroupDerivedData.objects.filter(id=old_id).exists() + + def test_cleanup_stale_processing_rows(self) -> None: + group = self.create_group() + row = create_processing_row(group.id) + + assert cleanup_stale_processing_rows(max_age=timedelta(hours=1)) == 0 + + GroupDerivedData.objects.filter(id=row.id).update( + date_added=row.date_added - timedelta(hours=2) + ) + + assert cleanup_stale_processing_rows(max_age=timedelta(hours=1)) == 1 + assert not GroupDerivedData.objects.filter(id=row.id).exists() + + def test_drain_log_respects_time_limit(self) -> None: + group = self.create_group() + for _ in range(5): + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + row = create_processing_row(group.id) + + # Zero time limit forces bail after first batch + drained = processing._drain_log(row, batch_size=2, time_limit=timedelta(0)) + assert not drained + # Processed one batch (2 entries) but not all 5 + assert row.cursor_id > 0 + entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) + assert row.cursor_id < entries[-1].id + + def test_process_group_log_reenqueues_on_partial_drain(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + with ( + patch("sentry.issues.derived.processing._drain_log", return_value=False), + patch("sentry.issues.derived.tasks.process_group_log_task") as mock_task, + ): + derived = process_group_log(group.id) + + assert derived is not None + mock_task.delay.assert_called_once_with(group.id) + + def test_build_and_promote_exhaustion_deletes_row(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + # Create a live row, then keep advancing its cursor so the candidate + # can never catch up (simulated by patching promote_to_live). + process_group_log(group.id) + + with patch( + "sentry.issues.derived.processing.promote_to_live", + return_value=PromotionResult.CURSOR_BEHIND, + ): + build_and_promote_derived_data(group.id) + + # The non-live candidate was cleaned up + assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=False).exists() + # The original live row is untouched + assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + + def test_build_and_promote_raises_on_partial_drain(self) -> None: + group = self.create_group() + for _ in range(5): + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + with patch("sentry.issues.derived.processing._drain_log", return_value=False): + with pytest.raises(GroupLogTimeout) as exc_info: + build_and_promote_derived_data(group.id) + + assert exc_info.value.group_id == group.id + assert exc_info.value.derived_id is not None + + # The non-live row still exists for resumption + derived_id = exc_info.value.derived_id + row = GroupDerivedData.objects.get(id=derived_id, is_live=False) + assert row is not None + + # Resuming with that derived_id completes the promotion + build_and_promote_derived_data(group.id, derived_id=derived_id) + promoted = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert promoted.id == row.id + assert promoted.view_count == 5 + + def test_build_and_promote_breaks_on_superseded(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) + + with patch( + "sentry.issues.derived.processing.promote_to_live", + return_value=PromotionResult.SUPERSEDED, + ) as mock_promote: + build_and_promote_derived_data(group.id) + + # Should only try once, not retry + assert mock_promote.call_count == 1 + # --- Pure Python tests (no DB) --- @@ -605,6 +865,7 @@ def test_load_returns_defaults_for_empty_data(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, + is_live=True, data={}, ) state = GroupDerivedDataStore.load(PIPELINE, derived) @@ -615,6 +876,7 @@ def test_load_populates_columns_and_json(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, + is_live=True, view_count=3, progress="diagnosed", data={"status": "closed"}, @@ -629,6 +891,7 @@ def test_load_null_progress(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, + is_live=True, progress=None, data={}, ) @@ -642,6 +905,7 @@ def test_round_trip_preserves_state(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) first = process_group_log(group.id) + assert first is not None first_data = first.data.copy() first_view_count = first.view_count @@ -650,6 +914,7 @@ def test_round_trip_preserves_state(self) -> None: invalidate_group_derived_data(group.id) second = process_group_log(group.id) + assert second is not None assert second.data == first_data assert second.view_count == first_view_count @@ -693,6 +958,7 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=ViewAction(), actor=actor) derived = process_group_log(group.id) + assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] == IssueProgressState.IDENTIFIED @@ -700,6 +966,7 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=ResolveAction(), actor=actor) derived = process_group_log(group.id) + assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] is None @@ -707,6 +974,7 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=UnresolveAction(), actor=actor) derived = process_group_log(group.id) + assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] == IssueProgressState.IDENTIFIED @@ -719,8 +987,6 @@ class _IntentionalRollback(Exception): @with_feature("projects:issue-action-log-write-to-db") class DerivedDataTransactionTest(TestCase): - """Verify derived data processing respects transaction boundaries.""" - def test_rolled_back_action_does_not_produce_derived_data(self) -> None: group = self.create_group() @@ -752,8 +1018,8 @@ def test_committed_action_produces_derived_data(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) assert GroupActionLogEntry.objects.filter(group_id=group.id).count() == 1 - assert GroupDerivedData.objects.filter(group_id=group.id).exists() - derived = GroupDerivedData.objects.get(group_id=group.id) + assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + derived = GroupDerivedData.objects.get(group_id=group.id, is_live=True) assert derived.view_count == 1 @@ -775,4 +1041,5 @@ def test_completes_with_generous_timeout(self) -> None: GroupDerivedData.objects.filter(group_id=group.id).delete() derived = process_group_log(group.id, timeout=timedelta(minutes=5)) + assert derived is not None assert derived.view_count == 3 diff --git a/tests/sentry/issues/test_action_log.py b/tests/sentry/issues/test_action_log.py index 446092ab28e6..ac24f9b6b442 100644 --- a/tests/sentry/issues/test_action_log.py +++ b/tests/sentry/issues/test_action_log.py @@ -650,7 +650,7 @@ def test_flush_false_defers_drain(self) -> None: assert GroupActionLogEntry.objects.filter(group_id=self.group.id).count() == 1 - @patch("sentry.issues.derived.processing.process_group_log_task") + @patch("sentry.issues.derived.tasks.process_group_log_task") def test_force_async_derived_dispatches_task(self, mock_task: MagicMock) -> None: with self.feature("projects:issue-action-log-write-to-db"), outbox_runner(): publish_action( @@ -669,7 +669,7 @@ def test_force_async_derived_dispatches_task(self, mock_task: MagicMock) -> None # Task was dispatched instead mock_task.delay.assert_called_once_with(self.group.id) - @patch("sentry.issues.derived.processing.process_group_log_task") + @patch("sentry.issues.derived.tasks.process_group_log_task") def test_inline_derived_processes_without_task(self, mock_task: MagicMock) -> None: with self.feature("projects:issue-action-log-write-to-db"), outbox_runner(): publish_action( @@ -682,7 +682,7 @@ def test_inline_derived_processes_without_task(self, mock_task: MagicMock) -> No assert GroupActionLogEntry.objects.filter(group_id=self.group.id).count() == 1 # Derived data WAS processed inline - derived = GroupDerivedData.objects.get(group_id=self.group.id) + derived = GroupDerivedData.objects.get(group_id=self.group.id, is_live=True) assert derived.view_count == 1 # No async task needed (single entry = caught up) mock_task.delay.assert_not_called() diff --git a/tests/sentry/tasks/test_merge.py b/tests/sentry/tasks/test_merge.py index b03c8348b322..a2621079f82e 100644 --- a/tests/sentry/tasks/test_merge.py +++ b/tests/sentry/tasks/test_merge.py @@ -278,7 +278,7 @@ def test_merge_invalidates_derived_data(self) -> None: new_group = self.create_group(project) derived = GroupDerivedData.objects.create( - group=new_group, cursor_date=before_now(minutes=5), cursor_id=100 + group=new_group, is_live=True, cursor_date=before_now(minutes=5), cursor_id=100 ) with self.tasks(): @@ -288,7 +288,7 @@ def test_merge_invalidates_derived_data(self) -> None: # The derived data is invalidated: the stale row is deleted and rebuilt # from scratch by the scheduled processing task. - rebuilt = GroupDerivedData.objects.get(group_id=new_group.id) + rebuilt = GroupDerivedData.objects.get(group_id=new_group.id, is_live=True) assert rebuilt.id != derived.id assert rebuilt.cursor_date == EPOCH assert rebuilt.cursor_id == 0 From 1419d353a69697b58986803e6557400d7c24a717 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 17 Jul 2026 10:58:57 -0700 Subject: [PATCH 2/9] fixup --- tests/sentry/issues/derived/test_processing.py | 6 +++--- tests/sentry/issues/test_action_log.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index e1678dc09fd8..fb219d05d2a3 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -342,7 +342,7 @@ def test_invalidate_soft_schedules_rebuild(self) -> None: assert derived is not None original_id = derived.id - with patch("sentry.issues.derived.tasks.rebuild_group_derived_data_task") as mock_task: + with patch("sentry.issues.derived.processing.rebuild_group_derived_data_task") as mock_task: invalidate_group_derived_data(group.id, hard_delete=False) mock_task.delay.assert_called_once_with(group.id) @@ -489,7 +489,7 @@ def test_noop_when_on_demand_disabled(self) -> None: with self.options({"issues.derived-data.create-on-demand": False}): # Publish with async strategy so inline processing doesn't create a row - with patch("sentry.issues.derived.tasks.process_group_log_task"): + with patch("sentry.issues.derived.processing.process_group_log_task"): _publish( group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id) ) @@ -635,7 +635,7 @@ def test_process_group_log_reenqueues_on_partial_drain(self) -> None: with ( patch("sentry.issues.derived.processing._drain_log", return_value=False), - patch("sentry.issues.derived.tasks.process_group_log_task") as mock_task, + patch("sentry.issues.derived.processing.process_group_log_task") as mock_task, ): derived = process_group_log(group.id) diff --git a/tests/sentry/issues/test_action_log.py b/tests/sentry/issues/test_action_log.py index ac24f9b6b442..02cc3f94d558 100644 --- a/tests/sentry/issues/test_action_log.py +++ b/tests/sentry/issues/test_action_log.py @@ -650,7 +650,7 @@ def test_flush_false_defers_drain(self) -> None: assert GroupActionLogEntry.objects.filter(group_id=self.group.id).count() == 1 - @patch("sentry.issues.derived.tasks.process_group_log_task") + @patch("sentry.issues.derived.processing.process_group_log_task") def test_force_async_derived_dispatches_task(self, mock_task: MagicMock) -> None: with self.feature("projects:issue-action-log-write-to-db"), outbox_runner(): publish_action( @@ -669,7 +669,7 @@ def test_force_async_derived_dispatches_task(self, mock_task: MagicMock) -> None # Task was dispatched instead mock_task.delay.assert_called_once_with(self.group.id) - @patch("sentry.issues.derived.tasks.process_group_log_task") + @patch("sentry.issues.derived.processing.process_group_log_task") def test_inline_derived_processes_without_task(self, mock_task: MagicMock) -> None: with self.feature("projects:issue-action-log-write-to-db"), outbox_runner(): publish_action( From 05fc4d149d5a16bb1135d5509236029bcf7c5c93 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 17 Jul 2026 15:36:31 -0700 Subject: [PATCH 3/9] insert-based --- src/sentry/issues/derived/processing.py | 242 +++++++++++------- src/sentry/issues/models/groupderiveddata.py | 37 ++- .../1141_add_is_live_to_group_derived_data.py | 6 + .../sentry/issues/derived/test_processing.py | 156 +++++------ tests/sentry/issues/derived/test_tasks.py | 2 +- 5 files changed, 274 insertions(+), 169 deletions(-) diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index 116d7291ac46..5aefcb286e81 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -5,7 +5,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.db import IntegrityError, router, transaction -from django.db.models import Q +from django.db.models import Max, Q from sentry import options from sentry.issues.derived.aggregators import AGGREGATORS @@ -24,6 +24,18 @@ DEFAULT_BATCH_SIZE = 1000 INLINE_BATCH_SIZE = 100 +# Fields that constitute the derived state, used by promote_to_live upsert. +_STATE_FIELDS = ( + "generation_id", + "cursor_date", + "cursor_id", + "data", + "view_count", + "progress", + "last_progressed_at", + "pipeline_hash", +) + class ProcessingStrategy(enum.Enum): SYNC = "sync" # process all pending actions now @@ -53,10 +65,12 @@ def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | Non if not Group.objects.filter(id=group_id).exists(): raise Group.DoesNotExist(f"Group {group_id} does not exist") + generation_id = _current_generation_id(group_id) derived, _created = GroupDerivedData.objects.get_or_create( group_id=group_id, is_live=True, defaults={ + "generation_id": generation_id, "cursor_date": EPOCH, "cursor_id": 0, "data": {}, @@ -83,25 +97,23 @@ def _process_batch( p: Pipeline[GroupActionLogEntry], derived: GroupDerivedData, batch_size: int, + *, + persist: bool = True, ) -> bool: """ Process up to `batch_size` entries for a group. Updates derived in place. Returns True if there are more entries to process. - Concurrency: multiple callers may process the same row simultaneously. - Safety relies on two properties: - - 1. The action log is append-only and the pipeline is deterministic, so - any caller processing the same entries produces the same result. - 2. The UPDATE uses a cursor guard scoped to the specific row (by id) - and pipeline_hash that only succeeds if no other caller has already - advanced the cursor past our batch. If it fails (updated == 0), a - concurrent caller already wrote a superset of our work, so we - refresh and check if more remains. + When *persist* is True (default), the update is written to the database + with an optimistic concurrency guard scoped to the row's id and + generation_id. The generation_id check ensures that if a rebuild + promoted a new generation between our read and write, we discard our + work (which was computed from the old generation's state) and refresh. - This is an optimistic concurrency scheme — no locks are held, and the - last-writer-wins semantics are safe because all writers compute the - same deterministic result for overlapping entry ranges. + When *persist* is False, only the in-memory object is updated — the + caller is responsible for persisting the result (e.g. via + ``promote_to_live``). Used for background rebuilds that accumulate + state in memory and write once at the end. """ group_id = derived.group_id entries = _entries_after_cursor(group_id, derived.cursor_date, derived.cursor_id, batch_size) @@ -116,8 +128,14 @@ def _process_batch( last_id = last.id state_update = GroupDerivedDataStore.build_update(p, result) + if not persist: + derived.cursor_date = last_date + derived.cursor_id = last_id + GroupDerivedDataStore.apply_to_instance(derived, state_update) + return len(entries) == batch_size + updated = GroupDerivedData.objects.filter( - Q(id=derived.id) + Q(id=derived.id, generation_id=derived.generation_id) & (Q(cursor_date__lt=last_date) | Q(cursor_date=last_date, cursor_id__lte=last_id)) & Q(pipeline_hash=derived.pipeline_hash) ).update(cursor_date=last_date, cursor_id=last_id, **state_update) @@ -177,16 +195,20 @@ def _drain_log( batch_size: int = DEFAULT_BATCH_SIZE, pipeline: Pipeline[GroupActionLogEntry] | None = None, time_limit: timedelta = DEFAULT_TIME_LIMIT, + *, + persist: bool = True, ) -> bool: """Process pending log entries into *derived*, batching as needed. Returns True if all entries were processed, False if the time limit was reached and more entries remain. The limit is checked between batches, so a single slow batch can exceed it. + + When *persist* is False, batches update only the in-memory object. """ deadline = time.monotonic() + time_limit.total_seconds() p = pipeline or PIPELINE - while _process_batch(p, derived, batch_size): + while _process_batch(p, derived, batch_size, persist=persist): if time.monotonic() >= deadline: return False return True @@ -278,12 +300,17 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) # --------------------------------------------------------------------------- -# Non-live row lifecycle: create, build, promote, cleanup +# Non-live row lifecycle: build in memory, upsert live, cleanup # --------------------------------------------------------------------------- def create_processing_row(group_id: int) -> GroupDerivedData: - """Create a new non-live GroupDerivedData row for background processing.""" + """Create a new non-live GroupDerivedData row for background processing. + + Primarily used for saving partial progress on timeout so a subsequent + task can resume. For normal builds, prefer an unsaved in-memory instance + via ``build_and_promote_derived_data``. + """ return GroupDerivedData.objects.create( group_id=group_id, is_live=False, @@ -293,51 +320,76 @@ def create_processing_row(group_id: int) -> GroupDerivedData: ) +def _current_generation_id(group_id: int) -> int: + """Return the current log generation for a group. + + This is ``max(id)`` from the action log, which directly encodes the + log state at the moment a rebuild starts. The index on + ``(group_id, date_added, id)`` makes this an index-only backward scan. + """ + result = GroupActionLogEntry.objects.filter(group_id=group_id).aggregate(Max("id")) + return result["id__max"] or 0 + + class PromotionResult(enum.Enum): PROMOTED = "promoted" CURSOR_BEHIND = "cursor_behind" - SUPERSEDED = "superseded" - CANDIDATE_MISSING = "candidate_missing" - RACE_LOST = "race_lost" -class _PromotionAborted(Exception): - pass +def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: + """Upsert the candidate's state into the live row for its group. + If a live row exists and the candidate's generation and cursor are at + or ahead of the live row, the live row is updated in place. If no live + row exists, one is created. -def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: - """Atomically promote a non-live row to live, replacing any existing live row. + Returns CURSOR_BEHIND when the existing live row has a higher + generation_id (a newer rebuild supersedes us) or a more advanced cursor + within the same generation. - On success the old live row is deleted within the same transaction. - On any failure the transaction rolls back, restoring the old live row. + The candidate object itself is not modified or persisted — it may be an + unsaved in-memory instance used only to carry the computed state. """ + values = {f: getattr(candidate, f) for f in _STATE_FIELDS} + + # Try updating the existing live row. The guard requires that the + # candidate's generation_id is >= the live row's (so a stale rebuild + # can't overwrite a newer one), and within the same generation the + # cursor must be at or ahead. + updated = ( + GroupDerivedData.objects.filter( + group_id=candidate.group_id, + is_live=True, + ) + .filter( + Q(generation_id__lt=candidate.generation_id) + | Q( + generation_id=candidate.generation_id, + cursor_date__lt=candidate.cursor_date, + ) + | Q( + generation_id=candidate.generation_id, + cursor_date=candidate.cursor_date, + cursor_id__lte=candidate.cursor_id, + ) + ) + .update(**values) + ) + + if updated: + return PromotionResult.PROMOTED + + # No rows updated — either the live row's generation/cursor is ahead, + # or no live row exists yet. Try inserting; IntegrityError means a + # live row exists whose generation/cursor we didn't beat. try: - with transaction.atomic(using=router.db_for_write(GroupDerivedData)): - current_live = GroupDerivedData.objects.filter( - group_id=candidate.group_id, is_live=True - ).first() - - if current_live is not None: - # A candidate older than the live row means a concurrent or - # earlier build is trying to replace a newer one — reject it. - if candidate.id < current_live.id: - return PromotionResult.SUPERSEDED - if (candidate.cursor_date, candidate.cursor_id) < ( - current_live.cursor_date, - current_live.cursor_id, - ): - return PromotionResult.CURSOR_BEHIND - current_live.delete() - - updated = GroupDerivedData.objects.filter(id=candidate.id).update(is_live=True) - if not updated: - raise _PromotionAborted - - candidate.is_live = True - except _PromotionAborted: - return PromotionResult.CANDIDATE_MISSING + GroupDerivedData.objects.create( + group_id=candidate.group_id, + is_live=True, + **values, + ) except IntegrityError: - return PromotionResult.RACE_LOST + return PromotionResult.CURSOR_BEHIND return PromotionResult.PROMOTED @@ -345,74 +397,82 @@ def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: MAX_PROMOTION_ATTEMPTS = 5 -def _get_or_create_processing_row(group_id: int, derived_id: int | None) -> GroupDerivedData | None: - """Resume an existing non-live row by id, or create a new one. - - Returns None if creation fails (IntegrityError) or the requested row - no longer exists (cleaned up). - """ - if derived_id is not None: - return GroupDerivedData.objects.filter(id=derived_id, is_live=False).first() - try: - return create_processing_row(group_id) - except IntegrityError: - return None - - def build_and_promote_derived_data( group_id: int, batch_size: int = DEFAULT_BATCH_SIZE, derived_id: int | None = None, time_limit: timedelta = DEFAULT_TIME_LIMIT, ) -> None: - """Create (or resume) a non-live row, drain the log into it, and promote. + """Build derived data from scratch and upsert into the live row. + + The common path works entirely in memory: a transient GroupDerivedData + is populated by draining the action log, then its state is upserted + into the live row (2-3 queries total for single-batch groups). - When *derived_id* is provided, an existing non-live row is resumed instead - of creating a new one. This allows the task to be re-enqueued and pick up - where it left off after a time-limited drain. + When *derived_id* is provided, a previously persisted non-live row is + loaded and resumed. This happens after a prior run timed out and saved + its partial progress to the database. If promotion fails because the live row's cursor is ahead (it received incremental updates while we were building), we drain additional entries - to catch up and retry. This avoids discarding a rebuild that contains - corrected historical data just because the live row is more current. + to catch up and retry, bounded by MAX_PROMOTION_ATTEMPTS. - Retries are bounded to avoid starvation if the live row is being updated - faster than we can catch up. On exhaustion the caller should re-enqueue. - - Raises GroupLogTimeout (with ``derived_id`` set) if the time-limited drain - could not finish, so the caller can decide its own retry strategy. + Raises GroupLogTimeout (with ``derived_id`` set) if the time-limited + drain could not finish, so the caller can re-enqueue with the id. """ - derived = _get_or_create_processing_row(group_id, derived_id) - if derived is None: - logger.info( - "issues.derived.build_and_promote.no_row", - extra={"group_id": group_id, "derived_id": derived_id}, + if derived_id is not None: + derived = GroupDerivedData.objects.filter(id=derived_id, is_live=False).first() + if derived is None: + logger.info( + "issues.derived.build_and_promote.no_row", + extra={"group_id": group_id, "derived_id": derived_id}, + ) + return + else: + # Capture the current log generation before we start draining. + # This lets promote_to_live reject stale rebuilds that started + # before a later log mutation triggered a newer rebuild. + generation_id = _current_generation_id(group_id) + + # In-memory only — not saved to the database unless we time out. + derived = GroupDerivedData( + group_id=group_id, + is_live=False, + generation_id=generation_id, + cursor_date=EPOCH, + cursor_id=0, + data={}, + pipeline_hash=PIPELINE.pipeline_hash, ) - return result = PromotionResult.CURSOR_BEHIND for attempt in range(MAX_PROMOTION_ATTEMPTS): - drained = _drain_log(derived, batch_size, time_limit=time_limit) + drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - raise GroupLogTimeout(group_id, derived_id=derived.id) + # Save to DB for resumption if not already persisted. + if derived.pk is None: + derived.save() + raise GroupLogTimeout(group_id, derived_id=derived.pk) + result = promote_to_live(derived) if result is PromotionResult.PROMOTED: logger.info( "issues.derived.promoted", extra={ "group_id": group_id, - "derived_id": derived.id, "cursor_date": str(derived.cursor_date), "cursor_id": derived.cursor_id, "attempts": attempt + 1, }, ) + # Clean up the non-live row if it was persisted for resumption. + if derived.pk is not None: + derived.delete() return - if result is not PromotionResult.CURSOR_BEHIND: - break - - derived.delete() + # Clean up non-live row if persisted. + if derived.pk is not None: + derived.delete() if result is PromotionResult.CURSOR_BEHIND: metrics.incr("issues.derived.promotion_exhausted", sample_rate=1.0) @@ -420,7 +480,6 @@ def build_and_promote_derived_data( "issues.derived.promotion_exhausted", extra={ "group_id": group_id, - "derived_id": derived.id, "attempts": MAX_PROMOTION_ATTEMPTS, }, ) @@ -429,7 +488,6 @@ def build_and_promote_derived_data( "issues.derived.promotion_rejected", extra={ "group_id": group_id, - "derived_id": derived.id, "result": result.value, }, ) diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index 2e632e2bee55..37e452b09bce 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -26,9 +26,32 @@ class GroupDerivedData(DefaultFieldsModel): Multiple rows may exist per group, but at most one may be ``is_live=True`` at any time (enforced by a partial unique constraint). Only the live row is - considered canonical; non-live rows are transient build artifacts. - - See ``processing.py`` for the full lifecycle and promotion protocol. + considered canonical; non-live rows are transient build artifacts that + exist only when a rebuild times out and needs to be resumed. + + Update safety + ~~~~~~~~~~~~~ + The pipeline is deterministic: replaying the same log produces the same + state. However, the log is not strictly append-only — historical entries + may be inserted, which is a primary reason rebuilds are triggered. Two + guards on the live row prevent stale writes: + + * **generation_id** — set to ``max(GroupActionLogEntry.id)`` when a + rebuild starts, capturing the log state the rebuild observed. A write + only succeeds if its generation_id is >= the live row's, so a slow + rebuild that started before a log mutation cannot overwrite results + from a newer rebuild that observed the corrected log. + + * **cursor guard** — within the same generation, a write only succeeds + if the writer's ``(cursor_date, cursor_id)`` is at or ahead of the + live row's, preventing cursor regression. + + Incremental processing (live-row path) writes per-batch with the cursor + guard scoped to the row's ``id``, ``generation_id``, and + ``pipeline_hash``. Rebuilds accumulate state in memory and write once + via ``promote_to_live``, which uses the generation and cursor guards. + + See ``processing.py`` for the full lifecycle. """ __relocation_scope__ = RelocationScope.Excluded @@ -36,6 +59,12 @@ class GroupDerivedData(DefaultFieldsModel): group = FlexibleForeignKey("sentry.Group") is_live = models.BooleanField(db_default=False, default=False) + # Identifies the log state this row was built from. Set to + # ``max(GroupActionLogEntry.id)`` at the start of a rebuild so that + # the promote guard can reject stale rebuilds that started before a + # later log mutation triggered a newer rebuild. + generation_id = BoundedBigIntegerField(default=0) + cursor_date = models.DateTimeField(default=EPOCH) cursor_id = BoundedBigIntegerField(default=0) @@ -88,4 +117,4 @@ class Meta: ), ] - __repr__ = sane_repr("group_id", "is_live", "cursor_date", "cursor_id") + __repr__ = sane_repr("group_id", "is_live", "generation_id", "cursor_date", "cursor_id") diff --git a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py index e66bb8946de6..4fbe6819cd65 100644 --- a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py +++ b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py @@ -1,6 +1,7 @@ # Generated by Django 5.2.14 on 2026-07-09 15:43 import django.db.models.deletion +import sentry.db.models.fields.bounded import sentry.db.models.fields.foreignkey from django.db import migrations, models @@ -40,6 +41,11 @@ class Migration(CheckedMigration): name="is_live", field=models.BooleanField(db_default=False, default=False), ), + migrations.AddField( + model_name="groupderiveddata", + name="generation_id", + field=sentry.db.models.fields.bounded.BoundedBigIntegerField(default=0), + ), migrations.AlterField( model_name="groupderiveddata", name="group", diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index fb219d05d2a3..2a52d6077ae7 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -46,6 +46,7 @@ PIPELINE, GroupLogTimeout, PromotionResult, + _current_generation_id, _entries_after_cursor, build_and_promote_derived_data, cleanup_stale_processing_rows, @@ -56,7 +57,7 @@ ) from sentry.issues.derived.store import GroupDerivedDataStore from sentry.issues.models.groupactionlogentry import GroupActionLogEntry -from sentry.issues.models.groupderiveddata import GroupDerivedData +from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData from sentry.issues.progress_state import IssueProgressState from sentry.models.group import Group from sentry.testutils.cases import TestCase @@ -505,20 +506,22 @@ def test_noop_when_on_demand_disabled(self) -> None: @with_feature("projects:issue-action-log-write-to-db") class PromoteToLiveTest(TestCase): - def test_promote_with_no_existing_live(self) -> None: + def test_promote_inserts_when_no_live_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - row = create_processing_row(group.id) - processing._drain_log(row) - assert promote_to_live(row) is PromotionResult.PROMOTED - assert row.is_live + gen = _current_generation_id(group.id) + candidate = GroupDerivedData( + group_id=group.id, generation_id=gen, cursor_date=EPOCH, cursor_id=0, data={} + ) + processing._drain_log(candidate, persist=False) + assert promote_to_live(candidate) is PromotionResult.PROMOTED - row.refresh_from_db() - assert row.is_live - assert row.view_count == 1 + live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert live.view_count == 1 + assert live.generation_id == gen - def test_promote_replaces_older_live(self) -> None: + def test_promote_updates_existing_live_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -528,32 +531,17 @@ def test_promote_replaces_older_live(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - new = create_processing_row(group.id) - processing._drain_log(new) - assert promote_to_live(new) is PromotionResult.PROMOTED - - assert not GroupDerivedData.objects.filter(id=old_id).exists() - assert new.is_live - assert new.view_count == 2 - - def test_promote_rejected_if_candidate_older(self) -> None: - group = self.create_group() - _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - - old_candidate = create_processing_row(group.id) - processing._drain_log(old_candidate) - - # Create and promote a newer candidate first - _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - new_candidate = create_processing_row(group.id) - processing._drain_log(new_candidate) - assert promote_to_live(new_candidate) is PromotionResult.PROMOTED - - assert promote_to_live(old_candidate) is PromotionResult.SUPERSEDED + gen = _current_generation_id(group.id) + candidate = GroupDerivedData( + group_id=group.id, generation_id=gen, cursor_date=EPOCH, cursor_id=0, data={} + ) + processing._drain_log(candidate, persist=False) + assert promote_to_live(candidate) is PromotionResult.PROMOTED - # New candidate is still live + # The existing live row was updated in place, not replaced. live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) - assert live.id == new_candidate.id + assert live.id == old_id + assert live.view_count == 2 def test_promote_rejected_if_cursor_behind(self) -> None: group = self.create_group() @@ -564,17 +552,44 @@ def test_promote_rejected_if_cursor_behind(self) -> None: live = process_group_log(group.id) assert live is not None - # Create a candidate that only processes the first entry - candidate = create_processing_row(group.id) - processing._process_batch(PIPELINE, candidate, batch_size=1) + # Build a candidate with the same generation as the live row + # but only process the first entry so the cursor is behind. + candidate = GroupDerivedData( + group_id=group.id, + generation_id=live.generation_id, + cursor_date=EPOCH, + cursor_id=0, + data={}, + ) + processing._process_batch(PIPELINE, candidate, batch_size=1, persist=False) - # Candidate cursor is behind the live row assert (candidate.cursor_date, candidate.cursor_id) < ( live.cursor_date, live.cursor_id, ) assert promote_to_live(candidate) is PromotionResult.CURSOR_BEHIND + def test_promote_rejected_if_stale_generation(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + # Capture generation before more entries arrive + stale_gen = _current_generation_id(group.id) + + # Build and promote with current generation + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + build_and_promote_derived_data(group.id) + live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert live.generation_id > stale_gen + + # A stale rebuild with the old generation can't overwrite even if + # it has the same cursor position. + stale_candidate = GroupDerivedData( + group_id=group.id, generation_id=stale_gen, cursor_date=EPOCH, cursor_id=0, data={} + ) + processing._drain_log(stale_candidate, persist=False) + assert promote_to_live(stale_candidate) is PromotionResult.CURSOR_BEHIND + def test_build_and_promote(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -585,7 +600,7 @@ def test_build_and_promote(self) -> None: assert derived.view_count == 1 assert derived.data["status"] == "closed" - def test_build_and_promote_replaces_stale_live(self) -> None: + def test_build_and_promote_updates_stale_live(self) -> None: group = self.create_group() user = self.user @@ -596,10 +611,19 @@ def test_build_and_promote_replaces_stale_live(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) build_and_promote_derived_data(group.id) - new = GroupDerivedData.objects.get(group_id=group.id, is_live=True) - assert new.id != old_id - assert new.view_count == 2 - assert not GroupDerivedData.objects.filter(id=old_id).exists() + live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + # Upsert updates the existing live row in place. + assert live.id == old_id + assert live.view_count == 2 + + def test_build_and_promote_no_non_live_rows_on_success(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + build_and_promote_derived_data(group.id) + # Common path never persists a non-live row. + assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=False).exists() + assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() def test_cleanup_stale_processing_rows(self) -> None: group = self.create_group() @@ -619,15 +643,19 @@ def test_drain_log_respects_time_limit(self) -> None: for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - row = create_processing_row(group.id) + candidate = GroupDerivedData( + group_id=group.id, generation_id=0, cursor_date=EPOCH, cursor_id=0, data={} + ) # Zero time limit forces bail after first batch - drained = processing._drain_log(row, batch_size=2, time_limit=timedelta(0)) + drained = processing._drain_log( + candidate, batch_size=2, time_limit=timedelta(0), persist=False + ) assert not drained # Processed one batch (2 entries) but not all 5 - assert row.cursor_id > 0 + assert candidate.cursor_id > 0 entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) - assert row.cursor_id < entries[-1].id + assert candidate.cursor_id < entries[-1].id def test_process_group_log_reenqueues_on_partial_drain(self) -> None: group = self.create_group() @@ -642,12 +670,10 @@ def test_process_group_log_reenqueues_on_partial_drain(self) -> None: assert derived is not None mock_task.delay.assert_called_once_with(group.id) - def test_build_and_promote_exhaustion_deletes_row(self) -> None: + def test_build_and_promote_exhaustion_leaves_live_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - # Create a live row, then keep advancing its cursor so the candidate - # can never catch up (simulated by patching promote_to_live). process_group_log(group.id) with patch( @@ -656,12 +682,12 @@ def test_build_and_promote_exhaustion_deletes_row(self) -> None: ): build_and_promote_derived_data(group.id) - # The non-live candidate was cleaned up + # No non-live rows persisted (common path is in-memory). assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=False).exists() - # The original live row is untouched + # The original live row is untouched. assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() - def test_build_and_promote_raises_on_partial_drain(self) -> None: + def test_build_and_promote_saves_on_timeout_for_resumption(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -673,30 +699,16 @@ def test_build_and_promote_raises_on_partial_drain(self) -> None: assert exc_info.value.group_id == group.id assert exc_info.value.derived_id is not None - # The non-live row still exists for resumption + # The non-live row was persisted for resumption. derived_id = exc_info.value.derived_id - row = GroupDerivedData.objects.get(id=derived_id, is_live=False) - assert row is not None + assert GroupDerivedData.objects.filter(id=derived_id, is_live=False).exists() - # Resuming with that derived_id completes the promotion + # Resuming with that derived_id completes the promotion. build_and_promote_derived_data(group.id, derived_id=derived_id) promoted = GroupDerivedData.objects.get(group_id=group.id, is_live=True) - assert promoted.id == row.id assert promoted.view_count == 5 - - def test_build_and_promote_breaks_on_superseded(self) -> None: - group = self.create_group() - _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - process_group_log(group.id) - - with patch( - "sentry.issues.derived.processing.promote_to_live", - return_value=PromotionResult.SUPERSEDED, - ) as mock_promote: - build_and_promote_derived_data(group.id) - - # Should only try once, not retry - assert mock_promote.call_count == 1 + # The non-live row was cleaned up after promotion. + assert not GroupDerivedData.objects.filter(id=derived_id, is_live=False).exists() # --- Pure Python tests (no DB) --- diff --git a/tests/sentry/issues/derived/test_tasks.py b/tests/sentry/issues/derived/test_tasks.py index a8b7bd6f8f32..77dfb79b0992 100644 --- a/tests/sentry/issues/derived/test_tasks.py +++ b/tests/sentry/issues/derived/test_tasks.py @@ -156,7 +156,7 @@ def test_reschedules_on_group_log_timeout(self) -> None: patch.object( processing, "process_group_log", - side_effect=GroupLogTimeout("timed out"), + side_effect=GroupLogTimeout(0), ), patch.object(process_project_derived_data_batch, "delay") as mock_delay, ): From d22a1118737e245b6f3ee5bedb214fc6e84f8e3f Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 17 Jul 2026 16:52:02 -0700 Subject: [PATCH 4/9] another pass --- src/sentry/issues/derived/processing.py | 117 ++++++++---------- src/sentry/issues/derived/tasks.py | 12 +- src/sentry/issues/models/groupderiveddata.py | 18 +-- .../1141_add_is_live_to_group_derived_data.py | 2 +- .../sentry/issues/derived/test_processing.py | 9 +- 5 files changed, 77 insertions(+), 81 deletions(-) diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index 5aefcb286e81..c38c77d6ad95 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -11,7 +11,7 @@ from sentry.issues.derived.aggregators import AGGREGATORS from sentry.issues.derived.framework import Pipeline from sentry.issues.derived.store import GroupDerivedDataStore -from sentry.issues.derived.tasks import process_group_log_task, rebuild_group_derived_data_task +from sentry.issues.derived.tasks import process_group_log_task, rebuild_group_derived_data from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData from sentry.models.group import Group @@ -24,16 +24,12 @@ DEFAULT_BATCH_SIZE = 1000 INLINE_BATCH_SIZE = 100 -# Fields that constitute the derived state, used by promote_to_live upsert. -_STATE_FIELDS = ( - "generation_id", - "cursor_date", - "cursor_id", - "data", - "view_count", - "progress", - "last_progressed_at", - "pipeline_hash", +# Fields that constitute the derived state, written by promote_to_live. +# Derived by excluding identity and metadata fields from the model — new +# columns are automatically included unless explicitly excluded here. +_IDENTITY_FIELDS = frozenset({"id", "group_id", "is_live", "date_added", "date_updated"}) +_STATE_FIELDS = tuple( + f.attname for f in GroupDerivedData._meta.concrete_fields if f.attname not in _IDENTITY_FIELDS ) @@ -105,10 +101,10 @@ def _process_batch( Returns True if there are more entries to process. When *persist* is True (default), the update is written to the database - with an optimistic concurrency guard scoped to the row's id and - generation_id. The generation_id check ensures that if a rebuild - promoted a new generation between our read and write, we discard our - work (which was computed from the old generation's state) and refresh. + with an optimistic concurrency guard that ensures the row's identity, + generation, pipeline version, and cursor position haven't changed since + we read it. If the guard fails (a concurrent writer or rebuild advanced + the row), we refresh from the database and check for remaining work. When *persist* is False, only the in-memory object is updated — the caller is responsible for persisting the result (e.g. via @@ -240,18 +236,12 @@ def process_group_log( if derived is None: return None - if timeout is not None: - timeout_seconds = timeout.total_seconds() - start = time.monotonic() - has_more = _process_batch(p, derived, batch_size) - while has_more: - if time.monotonic() - start >= timeout_seconds: - raise GroupLogTimeout(group_id) - has_more = _process_batch(p, derived, batch_size) - else: - drained = _drain_log(derived, batch_size, p) - if not drained: - process_group_log_task.delay(group_id) + time_limit = timeout if timeout is not None else DEFAULT_TIME_LIMIT + drained = _drain_log(derived, batch_size, p, time_limit=time_limit) + if not drained: + if timeout is not None: + raise GroupLogTimeout(group_id) + process_group_log_task.delay(group_id) return derived @@ -304,28 +294,11 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) # --------------------------------------------------------------------------- -def create_processing_row(group_id: int) -> GroupDerivedData: - """Create a new non-live GroupDerivedData row for background processing. - - Primarily used for saving partial progress on timeout so a subsequent - task can resume. For normal builds, prefer an unsaved in-memory instance - via ``build_and_promote_derived_data``. - """ - return GroupDerivedData.objects.create( - group_id=group_id, - is_live=False, - cursor_date=EPOCH, - cursor_id=0, - data={}, - ) - - def _current_generation_id(group_id: int) -> int: """Return the current log generation for a group. This is ``max(id)`` from the action log, which directly encodes the - log state at the moment a rebuild starts. The index on - ``(group_id, date_added, id)`` makes this an index-only backward scan. + log state at the moment a rebuild starts. """ result = GroupActionLogEntry.objects.filter(group_id=group_id).aggregate(Max("id")) return result["id__max"] or 0 @@ -333,7 +306,8 @@ def _current_generation_id(group_id: int) -> int: class PromotionResult(enum.Enum): PROMOTED = "promoted" - CURSOR_BEHIND = "cursor_behind" + SUPERSEDED = "superseded" # live row has a newer generation_id + CURSOR_BEHIND = "cursor_behind" # same generation, live cursor is ahead def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: @@ -343,9 +317,9 @@ def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: or ahead of the live row, the live row is updated in place. If no live row exists, one is created. - Returns CURSOR_BEHIND when the existing live row has a higher - generation_id (a newer rebuild supersedes us) or a more advanced cursor - within the same generation. + Returns SUPERSEDED when the live row has a higher generation_id (a + newer rebuild already won). Returns CURSOR_BEHIND when the generation + matches but the live row's cursor is more advanced. The candidate object itself is not modified or persisted — it may be an unsaved in-memory instance used only to carry the computed state. @@ -381,14 +355,26 @@ def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: # No rows updated — either the live row's generation/cursor is ahead, # or no live row exists yet. Try inserting; IntegrityError means a - # live row exists whose generation/cursor we didn't beat. + # live row exists whose generation/cursor we didn't beat. The + # savepoint ensures the IntegrityError doesn't poison the outer + # transaction, allowing the follow-up query to run. try: - GroupDerivedData.objects.create( - group_id=candidate.group_id, - is_live=True, - **values, - ) + with transaction.atomic(using=router.db_for_write(GroupDerivedData)): + GroupDerivedData.objects.create( + group_id=candidate.group_id, + is_live=True, + **values, + ) except IntegrityError: + # Distinguish: is the live row from a newer generation, or same + # generation with a more advanced cursor? + live_gen = ( + GroupDerivedData.objects.filter(group_id=candidate.group_id, is_live=True) + .values_list("generation_id", flat=True) + .first() + ) + if live_gen is not None and live_gen > candidate.generation_id: + return PromotionResult.SUPERSEDED return PromotionResult.CURSOR_BEHIND return PromotionResult.PROMOTED @@ -449,9 +435,10 @@ def build_and_promote_derived_data( for attempt in range(MAX_PROMOTION_ATTEMPTS): drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - # Save to DB for resumption if not already persisted. - if derived.pk is None: - derived.save() + # Persist current progress for resumption. For fresh builds + # this creates the row; for resumed builds it advances the + # cursor so the next run picks up where we left off. + derived.save() raise GroupLogTimeout(group_id, derived_id=derived.pk) result = promote_to_live(derived) @@ -470,6 +457,10 @@ def build_and_promote_derived_data( derived.delete() return + if result is PromotionResult.SUPERSEDED: + # A newer rebuild already won — retrying is futile. + break + # Clean up non-live row if persisted. if derived.pk is not None: derived.delete() @@ -480,12 +471,12 @@ def build_and_promote_derived_data( "issues.derived.promotion_exhausted", extra={ "group_id": group_id, - "attempts": MAX_PROMOTION_ATTEMPTS, + "attempts": attempt + 1, }, ) logger.info( - "issues.derived.promotion_rejected", + "issues.derived.promotion_failed", extra={ "group_id": group_id, "result": result.value, @@ -534,12 +525,12 @@ def invalidate_group_derived_data( ``hard_delete=True``. """ if not hard_delete: - rebuild_group_derived_data_task.delay(group_id) + rebuild_group_derived_data.delay(group_id) return if cursor is None: GroupDerivedData.objects.filter(group_id=group_id, is_live=True).delete() - rebuild_group_derived_data_task.delay(group_id) + rebuild_group_derived_data.delay(group_id) return # Only invalidate if the row has already processed past the affected point. @@ -557,4 +548,4 @@ def invalidate_group_derived_data( "cursor_id": cursor_id, }, ) - rebuild_group_derived_data_task.delay(group_id) + rebuild_group_derived_data.delay(group_id) diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 324ca83d7134..322864b03796 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -39,11 +39,11 @@ def process_group_log_task(group_id: int, **kwargs: object) -> None: @instrumented_task( - name="sentry.issues.derived.tasks.rebuild_group_derived_data_task", + name="sentry.issues.derived.tasks.rebuild_group_derived_data", namespace=issues_tasks, silo_mode=SiloMode.CELL, ) -def rebuild_group_derived_data_task( +def rebuild_group_derived_data( group_id: int, derived_id: int | None = None, prior_runs: int = 0, @@ -59,7 +59,7 @@ def rebuild_group_derived_data_task( activation_id = task_state.id if task_state else None if activation_id and already_spawned(_REBUILD_GROUP_TASK_KEY, activation_id): logger.info( - "rebuild_group_derived_data_task.duplicate_skipped", + "rebuild_group_derived_data.duplicate_skipped", extra={"group_id": group_id, "activation_id": activation_id}, ) metrics.incr( @@ -73,7 +73,7 @@ def rebuild_group_derived_data_task( except GroupLogTimeout as e: if prior_runs + 1 >= _MAX_REBUILD_RUNS: logger.error( - "rebuild_group_derived_data_task.max_runs_exceeded", + "rebuild_group_derived_data.max_runs_exceeded", extra={ "group_id": group_id, "derived_id": e.derived_id, @@ -82,7 +82,7 @@ def rebuild_group_derived_data_task( ) metrics.incr("issues.derived.rebuild_max_runs_exceeded", sample_rate=1.0) return - rebuild_group_derived_data_task.delay( + rebuild_group_derived_data.delay( group_id, derived_id=e.derived_id, prior_runs=prior_runs + 1 ) if activation_id: @@ -409,7 +409,7 @@ def rebuild_project_derived_data_batch( except GroupLogTimeout as e: # Re-enqueue the single group with its id so the # partially-drained row is resumed, then continue the batch. - rebuild_group_derived_data_task.delay(group_id, derived_id=e.derived_id) + rebuild_group_derived_data.delay(group_id, derived_id=e.derived_id) processed += 1 if time.monotonic() - start >= timeout_seconds: diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index 37e452b09bce..9630a12d5650 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -33,8 +33,8 @@ class GroupDerivedData(DefaultFieldsModel): ~~~~~~~~~~~~~ The pipeline is deterministic: replaying the same log produces the same state. However, the log is not strictly append-only — historical entries - may be inserted, which is a primary reason rebuilds are triggered. Two - guards on the live row prevent stale writes: + may be inserted, which is a primary reason rebuilds are triggered. + Three guards on the live row prevent stale writes: * **generation_id** — set to ``max(GroupActionLogEntry.id)`` when a rebuild starts, capturing the log state the rebuild observed. A write @@ -46,10 +46,14 @@ class GroupDerivedData(DefaultFieldsModel): if the writer's ``(cursor_date, cursor_id)`` is at or ahead of the live row's, preventing cursor regression. - Incremental processing (live-row path) writes per-batch with the cursor - guard scoped to the row's ``id``, ``generation_id``, and - ``pipeline_hash``. Rebuilds accumulate state in memory and write once - via ``promote_to_live``, which uses the generation and cursor guards. + * **pipeline_hash** — stamped at row creation, incremental writes only + succeed if the pipeline version hasn't changed since the row was + read. A pipeline upgrade invalidates in-flight incremental work. + + Incremental processing writes per-batch with all three guards scoped + to the row's ``id``. Rebuilds accumulate state in memory and write + once via ``promote_to_live``, which uses the generation and cursor + guards. See ``processing.py`` for the full lifecycle. """ @@ -63,7 +67,7 @@ class GroupDerivedData(DefaultFieldsModel): # ``max(GroupActionLogEntry.id)`` at the start of a rebuild so that # the promote guard can reject stale rebuilds that started before a # later log mutation triggered a newer rebuild. - generation_id = BoundedBigIntegerField(default=0) + generation_id = BoundedBigIntegerField(db_default=0, default=0) cursor_date = models.DateTimeField(default=EPOCH) cursor_id = BoundedBigIntegerField(default=0) diff --git a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py index 4fbe6819cd65..30852ccf3b54 100644 --- a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py +++ b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py @@ -44,7 +44,7 @@ class Migration(CheckedMigration): migrations.AddField( model_name="groupderiveddata", name="generation_id", - field=sentry.db.models.fields.bounded.BoundedBigIntegerField(default=0), + field=sentry.db.models.fields.bounded.BoundedBigIntegerField(db_default=0, default=0), ), migrations.AlterField( model_name="groupderiveddata", diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index 2a52d6077ae7..06d82ee57b45 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -50,7 +50,6 @@ _entries_after_cursor, build_and_promote_derived_data, cleanup_stale_processing_rows, - create_processing_row, invalidate_group_derived_data, process_group_log, promote_to_live, @@ -343,7 +342,7 @@ def test_invalidate_soft_schedules_rebuild(self) -> None: assert derived is not None original_id = derived.id - with patch("sentry.issues.derived.processing.rebuild_group_derived_data_task") as mock_task: + with patch("sentry.issues.derived.processing.rebuild_group_derived_data") as mock_task: invalidate_group_derived_data(group.id, hard_delete=False) mock_task.delay.assert_called_once_with(group.id) @@ -588,7 +587,7 @@ def test_promote_rejected_if_stale_generation(self) -> None: group_id=group.id, generation_id=stale_gen, cursor_date=EPOCH, cursor_id=0, data={} ) processing._drain_log(stale_candidate, persist=False) - assert promote_to_live(stale_candidate) is PromotionResult.CURSOR_BEHIND + assert promote_to_live(stale_candidate) is PromotionResult.SUPERSEDED def test_build_and_promote(self) -> None: group = self.create_group() @@ -627,7 +626,9 @@ def test_build_and_promote_no_non_live_rows_on_success(self) -> None: def test_cleanup_stale_processing_rows(self) -> None: group = self.create_group() - row = create_processing_row(group.id) + row = GroupDerivedData.objects.create( + group_id=group.id, is_live=False, cursor_date=EPOCH, cursor_id=0, data={} + ) assert cleanup_stale_processing_rows(max_age=timedelta(hours=1)) == 0 From f7513063cdbfe856e1327f20753c9f98fe78ea62 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 17 Jul 2026 17:31:37 -0700 Subject: [PATCH 5/9] more tests --- .../sentry/issues/derived/test_processing.py | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index 06d82ee57b45..dfe18fb1028a 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -474,6 +474,34 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: assert derived.cursor_id == first_cursor assert derived.pipeline_hash == "reset" + def test_generation_id_change_skips_incremental_write(self) -> None: + group = self.create_group() + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + derived = process_group_log(group.id) + assert derived is not None + first_cursor = derived.cursor_id + + GroupActionLogEntry.objects.create( + group_id=group.id, + project_id=group.project_id, + type=GroupActionType.VIEW, + actor_type=GroupActorType.SYSTEM, + actor_id=0, + source=SOURCE, + data={}, + ) + + # Simulate a rebuild promoting a new generation between our read + # and the UPDATE in _process_batch. + GroupDerivedData.objects.filter(id=derived.id).update(generation_id=999999) + + processing._process_batch(processing.PIPELINE, derived, 1) + + derived.refresh_from_db() + assert derived.cursor_id == first_cursor + assert derived.generation_id == 999999 + def test_invalidate_and_reprocess_restores_pipeline_hash(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -711,6 +739,30 @@ def test_build_and_promote_saves_on_timeout_for_resumption(self) -> None: # The non-live row was cleaned up after promotion. assert not GroupDerivedData.objects.filter(id=derived_id, is_live=False).exists() + def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: + group = self.create_group() + for _ in range(5): + _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + + # First run: drain 2 entries then timeout. + with pytest.raises(GroupLogTimeout) as exc_info: + build_and_promote_derived_data(group.id, batch_size=2, time_limit=timedelta(0)) + + derived_id = exc_info.value.derived_id + assert derived_id is not None + first_cursor = GroupDerivedData.objects.get(id=derived_id).cursor_id + assert first_cursor > 0 + + # Second run resumes and times out again — cursor must advance. + with pytest.raises(GroupLogTimeout) as exc_info: + build_and_promote_derived_data( + group.id, derived_id=derived_id, batch_size=2, time_limit=timedelta(0) + ) + + assert exc_info.value.derived_id == derived_id + second_cursor = GroupDerivedData.objects.get(id=derived_id).cursor_id + assert second_cursor > first_cursor + # --- Pure Python tests (no DB) --- From d3683769e1a94ff967d41baf4a6d3d57417cde44 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 17 Jul 2026 22:23:14 -0700 Subject: [PATCH 6/9] cache --- migrations_lockfile.txt | 2 +- src/sentry/issues/derived/processing.py | 164 +++++++++--------- src/sentry/issues/derived/tasks.py | 34 +++- src/sentry/issues/models/groupderiveddata.py | 43 +---- ...add_generation_id_to_group_derived_data.py | 22 +++ .../1141_add_is_live_to_group_derived_data.py | 92 ---------- .../sentry/issues/derived/test_processing.py | 93 ++++------ tests/sentry/issues/test_action_log.py | 2 +- tests/sentry/tasks/test_merge.py | 4 +- 9 files changed, 180 insertions(+), 276 deletions(-) create mode 100644 src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py delete mode 100644 src/sentry/migrations/1141_add_is_live_to_group_derived_data.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index 3307f526f448..de202241205f 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -31,7 +31,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0027_add_seer_run_coding_agent_handoff -sentry: 1141_add_is_live_to_group_derived_data +sentry: 1141_add_generation_id_to_group_derived_data social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index c38c77d6ad95..96bb25880f22 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -1,7 +1,8 @@ import enum import logging import time -from datetime import UTC, datetime, timedelta +from datetime import datetime, timedelta +from typing import NamedTuple from django.core.exceptions import ObjectDoesNotExist from django.db import IntegrityError, router, transaction @@ -16,6 +17,7 @@ from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData from sentry.models.group import Group from sentry.utils import metrics +from sentry.workflow_engine.caches.mapping import CacheMapping logger = logging.getLogger(__name__) @@ -27,12 +29,28 @@ # Fields that constitute the derived state, written by promote_to_live. # Derived by excluding identity and metadata fields from the model — new # columns are automatically included unless explicitly excluded here. -_IDENTITY_FIELDS = frozenset({"id", "group_id", "is_live", "date_added", "date_updated"}) +_IDENTITY_FIELDS = frozenset({"id", "group_id", "date_added", "date_updated"}) _STATE_FIELDS = tuple( f.attname for f in GroupDerivedData._meta.concrete_fields if f.attname not in _IDENTITY_FIELDS ) +class RebuildId(NamedTuple): + """Uniquely identifies a rebuild attempt for a group.""" + + group_id: int + generation_id: int + pipeline_hash: str + + +# Cache for in-progress rebuild state. +_rebuild_cache = CacheMapping[RebuildId, GroupDerivedData]( + lambda k: f"{k.group_id}:{k.generation_id}:{k.pipeline_hash}", + namespace="gdd-rebuild", + ttl_seconds=86400, +) + + class ProcessingStrategy(enum.Enum): SYNC = "sync" # process all pending actions now ASYNC = "async" # schedule a task to process all pending actions @@ -40,18 +58,16 @@ class ProcessingStrategy(enum.Enum): def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | None: - """Get the live GroupDerivedData row for a group, optionally creating one. + """Get the GroupDerivedData row for a group, optionally creating one. - When ``issues.derived-data.create-on-demand`` is enabled, a live row is + When ``issues.derived-data.create-on-demand`` is enabled, a row is created if none exists. When disabled, returns None — processing will be a no-op until a backfill task creates and promotes a row. Raises Group.DoesNotExist if the group has been deleted. """ - # Fast path: read-only check avoids a write attempt when the row exists - # or when on-demand creation is disabled. try: - return GroupDerivedData.objects.get(group_id=group_id, is_live=True) + return GroupDerivedData.objects.get(group_id=group_id) except GroupDerivedData.DoesNotExist: pass @@ -64,7 +80,6 @@ def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | Non generation_id = _current_generation_id(group_id) derived, _created = GroupDerivedData.objects.get_or_create( group_id=group_id, - is_live=True, defaults={ "generation_id": generation_id, "cursor_date": EPOCH, @@ -177,9 +192,9 @@ def _process_batch( class GroupLogTimeout(Exception): """Raised when processing cannot finish within its time budget.""" - def __init__(self, group_id: int, derived_id: int | None = None) -> None: + def __init__(self, group_id: int, rebuild_id: RebuildId | None = None) -> None: self.group_id = group_id - self.derived_id = derived_id + self.rebuild_id = rebuild_id super().__init__(group_id) @@ -221,9 +236,9 @@ def process_group_log( pipeline: Pipeline[GroupActionLogEntry] | None = None, timeout: timedelta | None = None, ) -> GroupDerivedData | None: - """Fully drain all pending entries for a group's live row. + """Fully drain all pending entries for a group's row. - Returns None if no live row exists and on-demand creation is disabled. + Returns None if no row exists and on-demand creation is disabled. Raises Group.DoesNotExist if the group has been deleted. Raises GroupLogTimeout if *timeout* elapses before all entries are processed. @@ -249,7 +264,7 @@ def process_group_log( def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) -> None: """Trigger derived data processing for a group. - Silently returns if the group has been deleted or no live row exists. + Silently returns if the group has been deleted or no row exists. Strategy controls how processing is dispatched: SYNC — process all pending actions now @@ -290,7 +305,7 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) # --------------------------------------------------------------------------- -# Non-live row lifecycle: build in memory, upsert live, cleanup +# Rebuild lifecycle: build in memory, upsert live, cache partial progress # --------------------------------------------------------------------------- @@ -304,36 +319,44 @@ def _current_generation_id(group_id: int) -> int: return result["id__max"] or 0 +def _save_rebuild_state(rebuild_id: RebuildId, derived: GroupDerivedData) -> None: + """Persist in-progress rebuild state to cache for later resumption.""" + _rebuild_cache.set(rebuild_id, derived) + + +def _load_rebuild_state(rebuild_id: RebuildId) -> GroupDerivedData | None: + """Load in-progress rebuild state from cache into an unsaved instance.""" + return _rebuild_cache.get(rebuild_id) + + class PromotionResult(enum.Enum): PROMOTED = "promoted" - SUPERSEDED = "superseded" # live row has a newer generation_id - CURSOR_BEHIND = "cursor_behind" # same generation, live cursor is ahead + SUPERSEDED = "superseded" # row has a newer generation_id + CURSOR_BEHIND = "cursor_behind" # same generation, cursor is ahead def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: - """Upsert the candidate's state into the live row for its group. + """Upsert the candidate's state into the row for its group. - If a live row exists and the candidate's generation and cursor are at - or ahead of the live row, the live row is updated in place. If no live - row exists, one is created. + If a row exists and the candidate's generation and cursor are at + or ahead, the row is updated in place. If no row exists, one is created. - Returns SUPERSEDED when the live row has a higher generation_id (a + Returns SUPERSEDED when the row has a higher generation_id (a newer rebuild already won). Returns CURSOR_BEHIND when the generation - matches but the live row's cursor is more advanced. + matches but the row's cursor is more advanced. The candidate object itself is not modified or persisted — it may be an unsaved in-memory instance used only to carry the computed state. """ values = {f: getattr(candidate, f) for f in _STATE_FIELDS} - # Try updating the existing live row. The guard requires that the - # candidate's generation_id is >= the live row's (so a stale rebuild + # Try updating the existing row. The guard requires that the + # candidate's generation_id is >= the row's (so a stale rebuild # can't overwrite a newer one), and within the same generation the # cursor must be at or ahead. updated = ( GroupDerivedData.objects.filter( group_id=candidate.group_id, - is_live=True, ) .filter( Q(generation_id__lt=candidate.generation_id) @@ -353,23 +376,22 @@ def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: if updated: return PromotionResult.PROMOTED - # No rows updated — either the live row's generation/cursor is ahead, - # or no live row exists yet. Try inserting; IntegrityError means a - # live row exists whose generation/cursor we didn't beat. The + # No rows updated — either the row's generation/cursor is ahead, + # or no row exists yet. Try inserting; IntegrityError means a + # row exists whose generation/cursor we didn't beat. The # savepoint ensures the IntegrityError doesn't poison the outer # transaction, allowing the follow-up query to run. try: with transaction.atomic(using=router.db_for_write(GroupDerivedData)): GroupDerivedData.objects.create( group_id=candidate.group_id, - is_live=True, **values, ) except IntegrityError: - # Distinguish: is the live row from a newer generation, or same + # Distinguish: is the row from a newer generation, or same # generation with a more advanced cursor? live_gen = ( - GroupDerivedData.objects.filter(group_id=candidate.group_id, is_live=True) + GroupDerivedData.objects.filter(group_id=candidate.group_id) .values_list("generation_id", flat=True) .first() ) @@ -386,60 +408,62 @@ def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: def build_and_promote_derived_data( group_id: int, batch_size: int = DEFAULT_BATCH_SIZE, - derived_id: int | None = None, + rebuild_id: RebuildId | None = None, time_limit: timedelta = DEFAULT_TIME_LIMIT, ) -> None: - """Build derived data from scratch and upsert into the live row. + """Build derived data from scratch and upsert into the row. The common path works entirely in memory: a transient GroupDerivedData is populated by draining the action log, then its state is upserted - into the live row (2-3 queries total for single-batch groups). + into the row (2-3 queries total for single-batch groups). - When *derived_id* is provided, a previously persisted non-live row is - loaded and resumed. This happens after a prior run timed out and saved - its partial progress to the database. + When *rebuild_id* is provided, previously cached partial progress is + loaded and resumed. This happens after a prior run timed out and + saved its state to cache. - If promotion fails because the live row's cursor is ahead (it received + If promotion fails because the row's cursor is ahead (it received incremental updates while we were building), we drain additional entries to catch up and retry, bounded by MAX_PROMOTION_ATTEMPTS. - Raises GroupLogTimeout (with ``derived_id`` set) if the time-limited - drain could not finish, so the caller can re-enqueue with the id. + Raises GroupLogTimeout (with ``rebuild_id`` set) if the time-limited + drain could not finish, so the caller can re-enqueue. """ - if derived_id is not None: - derived = GroupDerivedData.objects.filter(id=derived_id, is_live=False).first() + derived: GroupDerivedData | None = None + if rebuild_id is not None: + derived = _load_rebuild_state(rebuild_id) if derived is None: logger.info( - "issues.derived.build_and_promote.no_row", - extra={"group_id": group_id, "derived_id": derived_id}, + "issues.derived.build_and_promote.cache_miss", + extra={"group_id": group_id, "rebuild_id": rebuild_id}, ) - return - else: + + if derived is None: # Capture the current log generation before we start draining. # This lets promote_to_live reject stale rebuilds that started # before a later log mutation triggered a newer rebuild. generation_id = _current_generation_id(group_id) + pipeline_hash = PIPELINE.pipeline_hash + rebuild_id = RebuildId(group_id, generation_id, pipeline_hash) - # In-memory only — not saved to the database unless we time out. + # In-memory only — cached on timeout for resumption. derived = GroupDerivedData( group_id=group_id, - is_live=False, generation_id=generation_id, cursor_date=EPOCH, cursor_id=0, data={}, - pipeline_hash=PIPELINE.pipeline_hash, + pipeline_hash=pipeline_hash, ) + else: + rebuild_id = RebuildId(group_id, derived.generation_id, derived.pipeline_hash or "") result = PromotionResult.CURSOR_BEHIND + attempt = 0 for attempt in range(MAX_PROMOTION_ATTEMPTS): drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - # Persist current progress for resumption. For fresh builds - # this creates the row; for resumed builds it advances the - # cursor so the next run picks up where we left off. - derived.save() - raise GroupLogTimeout(group_id, derived_id=derived.pk) + _save_rebuild_state(rebuild_id, derived) + raise GroupLogTimeout(group_id, rebuild_id=rebuild_id) result = promote_to_live(derived) if result is PromotionResult.PROMOTED: @@ -452,18 +476,14 @@ def build_and_promote_derived_data( "attempts": attempt + 1, }, ) - # Clean up the non-live row if it was persisted for resumption. - if derived.pk is not None: - derived.delete() + _rebuild_cache.delete(rebuild_id) return if result is PromotionResult.SUPERSEDED: # A newer rebuild already won — retrying is futile. break - # Clean up non-live row if persisted. - if derived.pk is not None: - derived.delete() + _rebuild_cache.delete(rebuild_id) if result is PromotionResult.CURSOR_BEHIND: metrics.incr("issues.derived.promotion_exhausted", sample_rate=1.0) @@ -484,18 +504,6 @@ def build_and_promote_derived_data( ) -def cleanup_stale_processing_rows( - max_age: timedelta = timedelta(days=2), -) -> int: - """Delete non-live rows older than *max_age* that were never promoted.""" - cutoff = datetime.now(UTC) - max_age - deleted, _ = GroupDerivedData.objects.filter( - is_live=False, - date_added__lt=cutoff, - ).delete() - return deleted - - # --------------------------------------------------------------------------- # Invalidation # --------------------------------------------------------------------------- @@ -511,15 +519,15 @@ def invalidate_group_derived_data( *hard_delete* controls the strategy: - - ``True`` (default): delete the live row immediately and kick off an + - ``True`` (default): delete the row immediately and kick off an async task to rebuild from scratch. Use this when the existing data is known to be wrong and must not be served. - - ``False``: leave the current live row in place and kick off a background - build-and-promote. The existing live row continues serving reads until + - ``False``: leave the current row in place and kick off a background + build-and-promote. The existing row continues serving reads until the replacement is ready. If *cursor* is ``(date_added, id)`` of the earliest affected entry, the - invalidation only fires when the live row's cursor is at or past that + invalidation only fires when the row's cursor is at or past that point; otherwise the mutation is still ahead of processing and no invalidation is needed. *cursor* is only meaningful with ``hard_delete=True``. @@ -529,14 +537,14 @@ def invalidate_group_derived_data( return if cursor is None: - GroupDerivedData.objects.filter(group_id=group_id, is_live=True).delete() + GroupDerivedData.objects.filter(group_id=group_id).delete() rebuild_group_derived_data.delay(group_id) return # Only invalidate if the row has already processed past the affected point. cursor_date, cursor_id = cursor deleted, _ = GroupDerivedData.objects.filter( - Q(group_id=group_id, is_live=True) + Q(group_id=group_id) & (Q(cursor_date__gt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id)), ).delete() if deleted: diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 322864b03796..00f4d8cc2c46 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -45,14 +45,19 @@ def process_group_log_task(group_id: int, **kwargs: object) -> None: ) def rebuild_group_derived_data( group_id: int, - derived_id: int | None = None, + resume_generation_id: int | None = None, + resume_pipeline_hash: str | None = None, prior_runs: int = 0, **kwargs: object, ) -> None: - """Build a new GroupDerivedData row from scratch and promote it to live.""" + """Build a new GroupDerivedData row from scratch and promote it.""" from taskbroker_client.state import current_task - from sentry.issues.derived.processing import GroupLogTimeout, build_and_promote_derived_data + from sentry.issues.derived.processing import ( + GroupLogTimeout, + RebuildId, + build_and_promote_derived_data, + ) from sentry.taskworker.selfchain_idempotency import already_spawned, mark_spawned task_state = current_task() @@ -68,22 +73,30 @@ def rebuild_group_derived_data( ) return + rebuild_id: RebuildId | None = None + if resume_generation_id is not None and resume_pipeline_hash is not None: + rebuild_id = RebuildId(group_id, resume_generation_id, resume_pipeline_hash) + try: - build_and_promote_derived_data(group_id, derived_id=derived_id) + build_and_promote_derived_data(group_id, rebuild_id=rebuild_id) except GroupLogTimeout as e: if prior_runs + 1 >= _MAX_REBUILD_RUNS: logger.error( "rebuild_group_derived_data.max_runs_exceeded", extra={ "group_id": group_id, - "derived_id": e.derived_id, + "rebuild_id": e.rebuild_id, "prior_runs": prior_runs + 1, }, ) metrics.incr("issues.derived.rebuild_max_runs_exceeded", sample_rate=1.0) return + rid = e.rebuild_id rebuild_group_derived_data.delay( - group_id, derived_id=e.derived_id, prior_runs=prior_runs + 1 + group_id, + resume_generation_id=rid.generation_id if rid else None, + resume_pipeline_hash=rid.pipeline_hash if rid else None, + prior_runs=prior_runs + 1, ) if activation_id: mark_spawned(_REBUILD_GROUP_TASK_KEY, activation_id) @@ -112,7 +125,7 @@ def process_project_derived_data(project_id: int, **kwargs: object) -> None: # TODO: support very large projects via paginated iteration group_ids = list( Group.objects.filter(project_id=project_id) - .exclude(Exists(GroupDerivedData.objects.filter(group_id=OuterRef("id"), is_live=True))) + .exclude(Exists(GroupDerivedData.objects.filter(group_id=OuterRef("id")))) .order_by("id") .values_list("id", flat=True)[:_MAX_PROJECT_GROUPS] ) @@ -409,7 +422,12 @@ def rebuild_project_derived_data_batch( except GroupLogTimeout as e: # Re-enqueue the single group with its id so the # partially-drained row is resumed, then continue the batch. - rebuild_group_derived_data.delay(group_id, derived_id=e.derived_id) + rid = e.rebuild_id + rebuild_group_derived_data.delay( + group_id, + resume_generation_id=rid.generation_id if rid else None, + resume_pipeline_hash=rid.pipeline_hash if rid else None, + ) processed += 1 if time.monotonic() - start >= timeout_seconds: diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index 9630a12d5650..49178db0f06a 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -23,28 +23,24 @@ class GroupDerivedData(DefaultFieldsModel): """ Materialized state derived from GroupActionLogEntry entries. - - Multiple rows may exist per group, but at most one may be ``is_live=True`` - at any time (enforced by a partial unique constraint). Only the live row is - considered canonical; non-live rows are transient build artifacts that - exist only when a rebuild times out and needs to be resumed. + One row per group (enforced by ``unique=True`` on the FK). Update safety ~~~~~~~~~~~~~ The pipeline is deterministic: replaying the same log produces the same state. However, the log is not strictly append-only — historical entries may be inserted, which is a primary reason rebuilds are triggered. - Three guards on the live row prevent stale writes: + Three guards on the row prevent stale writes: * **generation_id** — set to ``max(GroupActionLogEntry.id)`` when a rebuild starts, capturing the log state the rebuild observed. A write - only succeeds if its generation_id is >= the live row's, so a slow + only succeeds if its generation_id is >= the row's, so a slow rebuild that started before a log mutation cannot overwrite results from a newer rebuild that observed the corrected log. * **cursor guard** — within the same generation, a write only succeeds if the writer's ``(cursor_date, cursor_id)`` is at or ahead of the - live row's, preventing cursor regression. + row's, preventing cursor regression. * **pipeline_hash** — stamped at row creation, incremental writes only succeed if the pipeline version hasn't changed since the row was @@ -60,8 +56,7 @@ class GroupDerivedData(DefaultFieldsModel): __relocation_scope__ = RelocationScope.Excluded - group = FlexibleForeignKey("sentry.Group") - is_live = models.BooleanField(db_default=False, default=False) + group = FlexibleForeignKey("sentry.Group", unique=True) # Identifies the log state this row was built from. Set to # ``max(GroupActionLogEntry.id)`` at the start of a rebuild so that @@ -94,31 +89,9 @@ class GroupDerivedData(DefaultFieldsModel): class Meta: app_label = "sentry" db_table = "sentry_groupderiveddata" - constraints = [ - models.UniqueConstraint( - fields=["group"], - condition=models.Q(is_live=True), - name="uniq_live_gdd_per_group", - ), - ] indexes = [ - # Only live rows participate in joins/filters on these columns. - models.Index( - fields=["progress", "group"], - condition=models.Q(is_live=True), - name="sentry_gdd_progress_live", - ), - models.Index( - fields=["last_progressed_at", "group"], - condition=models.Q(is_live=True), - name="sentry_gdd_lastprog_live", - ), - models.Index(fields=["group", "is_live"]), - models.Index( - fields=["date_added"], - condition=models.Q(is_live=False), - name="sentry_gdd_stale_cleanup", - ), + models.Index(fields=["progress", "group"]), + models.Index(fields=["last_progressed_at", "group"]), ] - __repr__ = sane_repr("group_id", "is_live", "generation_id", "cursor_date", "cursor_id") + __repr__ = sane_repr("group_id", "generation_id", "cursor_date", "cursor_id") diff --git a/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py b/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py new file mode 100644 index 000000000000..7b2e6689f98d --- /dev/null +++ b/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py @@ -0,0 +1,22 @@ +# Generated by Django 5.2.14 on 2026-07-09 15:43 + +import sentry.db.models.fields.bounded +from django.db import migrations + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + is_post_deployment = False + + dependencies = [ + ("sentry", "1140_organizationcontributors_unique_provider_hostname"), + ] + + operations = [ + migrations.AddField( + model_name="groupderiveddata", + name="generation_id", + field=sentry.db.models.fields.bounded.BoundedBigIntegerField(db_default=0, default=0), + ), + ] diff --git a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py b/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py deleted file mode 100644 index 30852ccf3b54..000000000000 --- a/src/sentry/migrations/1141_add_is_live_to_group_derived_data.py +++ /dev/null @@ -1,92 +0,0 @@ -# Generated by Django 5.2.14 on 2026-07-09 15:43 - -import django.db.models.deletion -import sentry.db.models.fields.bounded -import sentry.db.models.fields.foreignkey -from django.db import migrations, models - -from sentry.new_migrations.migrations import CheckedMigration - - -class Migration(CheckedMigration): - # This flag is used to mark that a migration shouldn't be automatically run in production. - # This should only be used for operations where it's safe to run the migration after your - # code has deployed. So this should not be used for most operations that alter the schema - # of a table. - # Here are some things that make sense to mark as post deployment: - # - Large data migrations. Typically we want these to be run manually so that they can be - # monitored and not block the deploy for a long period of time while they run. - # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to - # run this outside deployments so that we don't block them. Note that while adding an index - # is a schema change, it's completely safe to run the operation after the code has deployed. - # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment - - is_post_deployment = False - - dependencies = [ - ("sentry", "1140_organizationcontributors_unique_provider_hostname"), - ] - - operations = [ - migrations.RemoveIndex( - model_name="groupderiveddata", - name="sentry_grou_progres_f1627f_idx", - ), - migrations.RemoveIndex( - model_name="groupderiveddata", - name="sentry_grou_last_pr_8c8185_idx", - ), - migrations.AddField( - model_name="groupderiveddata", - name="is_live", - field=models.BooleanField(db_default=False, default=False), - ), - migrations.AddField( - model_name="groupderiveddata", - name="generation_id", - field=sentry.db.models.fields.bounded.BoundedBigIntegerField(db_default=0, default=0), - ), - migrations.AlterField( - model_name="groupderiveddata", - name="group", - field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( - on_delete=django.db.models.deletion.CASCADE, to="sentry.group" - ), - ), - migrations.AddIndex( - model_name="groupderiveddata", - index=models.Index( - condition=models.Q(("is_live", True)), - fields=["progress", "group"], - name="sentry_gdd_progress_live", - ), - ), - migrations.AddIndex( - model_name="groupderiveddata", - index=models.Index( - condition=models.Q(("is_live", True)), - fields=["last_progressed_at", "group"], - name="sentry_gdd_lastprog_live", - ), - ), - migrations.AddIndex( - model_name="groupderiveddata", - index=models.Index(fields=["group", "is_live"], name="sentry_grou_group_i_2bcdf6_idx"), - ), - migrations.AddIndex( - model_name="groupderiveddata", - index=models.Index( - condition=models.Q(("is_live", False)), - fields=["date_added"], - name="sentry_gdd_stale_cleanup", - ), - ), - migrations.AddConstraint( - model_name="groupderiveddata", - constraint=models.UniqueConstraint( - condition=models.Q(("is_live", True)), - fields=("group",), - name="uniq_live_gdd_per_group", - ), - ), - ] diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index dfe18fb1028a..0c3e582c5b2d 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -49,7 +49,6 @@ _current_generation_id, _entries_after_cursor, build_and_promote_derived_data, - cleanup_stale_processing_rows, invalidate_group_derived_data, process_group_log, promote_to_live, @@ -108,7 +107,6 @@ def test_records_and_processes(self) -> None: assert derived is not None assert derived.cursor_id == entries[-1].id assert isinstance(derived.data, dict) - assert derived.is_live def test_incremental_processing(self) -> None: group = self.create_group() @@ -145,7 +143,7 @@ def test_process_group_log_only_affects_target(self) -> None: _publish(group=group_a, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group_b, action=ViewAction(), actor=GroupActionActor.user(user.id)) - cursor_b = GroupDerivedData.objects.get(group_id=group_b.id, is_live=True).cursor_id + cursor_b = GroupDerivedData.objects.get(group_id=group_b.id).cursor_id GroupActionLogEntry.objects.create( group_id=group_a.id, @@ -158,7 +156,7 @@ def test_process_group_log_only_affects_target(self) -> None: ) process_group_log(group_a.id) - assert GroupDerivedData.objects.get(group_id=group_b.id, is_live=True).cursor_id == cursor_b + assert GroupDerivedData.objects.get(group_id=group_b.id).cursor_id == cursor_b def test_batched_processing(self) -> None: group = self.create_group() @@ -295,10 +293,10 @@ def test_invalidate_deletes_live_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) process_group_log(group.id) - assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + assert GroupDerivedData.objects.filter(group_id=group.id).exists() invalidate_group_derived_data(group.id) - assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + assert not GroupDerivedData.objects.filter(group_id=group.id).exists() def test_invalidate_with_cursor_deletes_if_past(self) -> None: group = self.create_group() @@ -307,7 +305,7 @@ def test_invalidate_with_cursor_deletes_if_past(self) -> None: assert derived is not None invalidate_group_derived_data(group.id, cursor=(derived.cursor_date, derived.cursor_id)) - assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + assert not GroupDerivedData.objects.filter(group_id=group.id).exists() def test_invalidate_with_cursor_noop_if_not_reached(self) -> None: group = self.create_group() @@ -349,7 +347,6 @@ def test_invalidate_soft_schedules_rebuild(self) -> None: # Live row is still in place derived.refresh_from_db() assert derived.id == original_id - assert derived.is_live def test_resolved_in_pull_request_proposes_fix(self) -> None: group = self.create_group() @@ -525,7 +522,7 @@ def test_noop_when_on_demand_disabled(self) -> None: derived = process_group_log(group.id) assert derived is None - assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + assert not GroupDerivedData.objects.filter(group_id=group.id).exists() # --- Non-live row lifecycle --- @@ -544,7 +541,7 @@ def test_promote_inserts_when_no_live_row(self) -> None: processing._drain_log(candidate, persist=False) assert promote_to_live(candidate) is PromotionResult.PROMOTED - live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + live = GroupDerivedData.objects.get(group_id=group.id) assert live.view_count == 1 assert live.generation_id == gen @@ -566,7 +563,7 @@ def test_promote_updates_existing_live_row(self) -> None: assert promote_to_live(candidate) is PromotionResult.PROMOTED # The existing live row was updated in place, not replaced. - live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + live = GroupDerivedData.objects.get(group_id=group.id) assert live.id == old_id assert live.view_count == 2 @@ -606,7 +603,7 @@ def test_promote_rejected_if_stale_generation(self) -> None: # Build and promote with current generation _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) build_and_promote_derived_data(group.id) - live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + live = GroupDerivedData.objects.get(group_id=group.id) assert live.generation_id > stale_gen # A stale rebuild with the old generation can't overwrite even if @@ -623,7 +620,7 @@ def test_build_and_promote(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(self.user.id)) build_and_promote_derived_data(group.id) - derived = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + derived = GroupDerivedData.objects.get(group_id=group.id) assert derived.view_count == 1 assert derived.data["status"] == "closed" @@ -638,7 +635,7 @@ def test_build_and_promote_updates_stale_live(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) build_and_promote_derived_data(group.id) - live = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + live = GroupDerivedData.objects.get(group_id=group.id) # Upsert updates the existing live row in place. assert live.id == old_id assert live.view_count == 2 @@ -648,24 +645,7 @@ def test_build_and_promote_no_non_live_rows_on_success(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) build_and_promote_derived_data(group.id) - # Common path never persists a non-live row. - assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=False).exists() - assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() - - def test_cleanup_stale_processing_rows(self) -> None: - group = self.create_group() - row = GroupDerivedData.objects.create( - group_id=group.id, is_live=False, cursor_date=EPOCH, cursor_id=0, data={} - ) - - assert cleanup_stale_processing_rows(max_age=timedelta(hours=1)) == 0 - - GroupDerivedData.objects.filter(id=row.id).update( - date_added=row.date_added - timedelta(hours=2) - ) - - assert cleanup_stale_processing_rows(max_age=timedelta(hours=1)) == 1 - assert not GroupDerivedData.objects.filter(id=row.id).exists() + assert GroupDerivedData.objects.filter(group_id=group.id).exists() def test_drain_log_respects_time_limit(self) -> None: group = self.create_group() @@ -711,12 +691,10 @@ def test_build_and_promote_exhaustion_leaves_live_row(self) -> None: ): build_and_promote_derived_data(group.id) - # No non-live rows persisted (common path is in-memory). - assert not GroupDerivedData.objects.filter(group_id=group.id, is_live=False).exists() - # The original live row is untouched. - assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() + # The original row is untouched. + assert GroupDerivedData.objects.filter(group_id=group.id).exists() - def test_build_and_promote_saves_on_timeout_for_resumption(self) -> None: + def test_build_and_promote_caches_on_timeout_for_resumption(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -726,20 +704,16 @@ def test_build_and_promote_saves_on_timeout_for_resumption(self) -> None: build_and_promote_derived_data(group.id) assert exc_info.value.group_id == group.id - assert exc_info.value.derived_id is not None - - # The non-live row was persisted for resumption. - derived_id = exc_info.value.derived_id - assert GroupDerivedData.objects.filter(id=derived_id, is_live=False).exists() + assert exc_info.value.rebuild_id is not None - # Resuming with that derived_id completes the promotion. - build_and_promote_derived_data(group.id, derived_id=derived_id) - promoted = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + # Resuming with that cache_key completes the promotion. + build_and_promote_derived_data(group.id, rebuild_id=exc_info.value.rebuild_id) + promoted = GroupDerivedData.objects.get(group_id=group.id) assert promoted.view_count == 5 - # The non-live row was cleaned up after promotion. - assert not GroupDerivedData.objects.filter(id=derived_id, is_live=False).exists() def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: + from sentry.issues.derived.processing import _load_rebuild_state + group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -748,20 +722,24 @@ def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data(group.id, batch_size=2, time_limit=timedelta(0)) - derived_id = exc_info.value.derived_id - assert derived_id is not None - first_cursor = GroupDerivedData.objects.get(id=derived_id).cursor_id + rid = exc_info.value.rebuild_id + assert rid is not None + state = _load_rebuild_state(rid) + assert state is not None + first_cursor = state.cursor_id assert first_cursor > 0 # Second run resumes and times out again — cursor must advance. with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data( - group.id, derived_id=derived_id, batch_size=2, time_limit=timedelta(0) + group.id, rebuild_id=rid, batch_size=2, time_limit=timedelta(0) ) - assert exc_info.value.derived_id == derived_id - second_cursor = GroupDerivedData.objects.get(id=derived_id).cursor_id - assert second_cursor > first_cursor + rid2 = exc_info.value.rebuild_id + assert rid2 is not None + state = _load_rebuild_state(rid2) + assert state is not None + assert state.cursor_id > first_cursor # --- Pure Python tests (no DB) --- @@ -930,7 +908,6 @@ def test_load_returns_defaults_for_empty_data(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, - is_live=True, data={}, ) state = GroupDerivedDataStore.load(PIPELINE, derived) @@ -941,7 +918,6 @@ def test_load_populates_columns_and_json(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, - is_live=True, view_count=3, progress="diagnosed", data={"status": "closed"}, @@ -956,7 +932,6 @@ def test_load_null_progress(self) -> None: group = self.create_group() derived = GroupDerivedData.objects.create( group=group, - is_live=True, progress=None, data={}, ) @@ -1083,8 +1058,8 @@ def test_committed_action_produces_derived_data(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) assert GroupActionLogEntry.objects.filter(group_id=group.id).count() == 1 - assert GroupDerivedData.objects.filter(group_id=group.id, is_live=True).exists() - derived = GroupDerivedData.objects.get(group_id=group.id, is_live=True) + assert GroupDerivedData.objects.filter(group_id=group.id).exists() + derived = GroupDerivedData.objects.get(group_id=group.id) assert derived.view_count == 1 diff --git a/tests/sentry/issues/test_action_log.py b/tests/sentry/issues/test_action_log.py index 02cc3f94d558..446092ab28e6 100644 --- a/tests/sentry/issues/test_action_log.py +++ b/tests/sentry/issues/test_action_log.py @@ -682,7 +682,7 @@ def test_inline_derived_processes_without_task(self, mock_task: MagicMock) -> No assert GroupActionLogEntry.objects.filter(group_id=self.group.id).count() == 1 # Derived data WAS processed inline - derived = GroupDerivedData.objects.get(group_id=self.group.id, is_live=True) + derived = GroupDerivedData.objects.get(group_id=self.group.id) assert derived.view_count == 1 # No async task needed (single entry = caught up) mock_task.delay.assert_not_called() diff --git a/tests/sentry/tasks/test_merge.py b/tests/sentry/tasks/test_merge.py index a2621079f82e..b03c8348b322 100644 --- a/tests/sentry/tasks/test_merge.py +++ b/tests/sentry/tasks/test_merge.py @@ -278,7 +278,7 @@ def test_merge_invalidates_derived_data(self) -> None: new_group = self.create_group(project) derived = GroupDerivedData.objects.create( - group=new_group, is_live=True, cursor_date=before_now(minutes=5), cursor_id=100 + group=new_group, cursor_date=before_now(minutes=5), cursor_id=100 ) with self.tasks(): @@ -288,7 +288,7 @@ def test_merge_invalidates_derived_data(self) -> None: # The derived data is invalidated: the stale row is deleted and rebuilt # from scratch by the scheduled processing task. - rebuilt = GroupDerivedData.objects.get(group_id=new_group.id, is_live=True) + rebuilt = GroupDerivedData.objects.get(group_id=new_group.id) assert rebuilt.id != derived.id assert rebuilt.cursor_date == EPOCH assert rebuilt.cursor_id == 0 From a7524b9a48e587fd2d206e28a42ae4e4218d3b44 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Mon, 20 Jul 2026 11:21:16 -0700 Subject: [PATCH 7/9] invalidated_at --- migrations_lockfile.txt | 2 +- src/sentry/issues/derived/processing.py | 271 ++++++++++-------- src/sentry/issues/derived/tasks.py | 25 +- src/sentry/issues/models/groupderiveddata.py | 47 +-- ...add_generation_id_to_group_derived_data.py | 22 -- ...dd_invalidated_at_to_group_derived_data.py | 29 ++ .../sentry/issues/derived/test_processing.py | 158 +++++----- tests/sentry/tasks/test_merge.py | 12 +- 8 files changed, 302 insertions(+), 264 deletions(-) delete mode 100644 src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py create mode 100644 src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index de202241205f..b41d6ad12ae8 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -31,7 +31,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0027_add_seer_run_coding_agent_handoff -sentry: 1141_add_generation_id_to_group_derived_data +sentry: 1141_add_invalidated_at_to_group_derived_data social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index 96bb25880f22..ac6872f94a39 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -6,7 +6,8 @@ from django.core.exceptions import ObjectDoesNotExist from django.db import IntegrityError, router, transaction -from django.db.models import Max, Q +from django.db.models import Q +from django.utils import timezone from sentry import options from sentry.issues.derived.aggregators import AGGREGATORS @@ -27,11 +28,11 @@ INLINE_BATCH_SIZE = 100 # Fields that constitute the derived state, written by promote_to_live. -# Derived by excluding identity and metadata fields from the model — new -# columns are automatically included unless explicitly excluded here. -_IDENTITY_FIELDS = frozenset({"id", "group_id", "date_added", "date_updated"}) +# Derived by excluding identity, control, and auto-managed fields from the +# model — new columns are automatically included unless explicitly excluded. +_EXCLUDED_FIELDS = frozenset({"id", "group_id", "invalidated_at", "date_added", "date_updated"}) _STATE_FIELDS = tuple( - f.attname for f in GroupDerivedData._meta.concrete_fields if f.attname not in _IDENTITY_FIELDS + f.attname for f in GroupDerivedData._meta.concrete_fields if f.attname not in _EXCLUDED_FIELDS ) @@ -39,13 +40,13 @@ class RebuildId(NamedTuple): """Uniquely identifies a rebuild attempt for a group.""" group_id: int - generation_id: int + invalidated_at: str # ISO timestamp or "deleted" for hard-delete rebuilds pipeline_hash: str # Cache for in-progress rebuild state. _rebuild_cache = CacheMapping[RebuildId, GroupDerivedData]( - lambda k: f"{k.group_id}:{k.generation_id}:{k.pipeline_hash}", + lambda k: f"{k.group_id}:{k.invalidated_at}:{k.pipeline_hash}", namespace="gdd-rebuild", ttl_seconds=86400, ) @@ -77,11 +78,9 @@ def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | Non if not Group.objects.filter(id=group_id).exists(): raise Group.DoesNotExist(f"Group {group_id} does not exist") - generation_id = _current_generation_id(group_id) derived, _created = GroupDerivedData.objects.get_or_create( group_id=group_id, defaults={ - "generation_id": generation_id, "cursor_date": EPOCH, "cursor_id": 0, "data": {}, @@ -117,9 +116,10 @@ def _process_batch( When *persist* is True (default), the update is written to the database with an optimistic concurrency guard that ensures the row's identity, - generation, pipeline version, and cursor position haven't changed since - we read it. If the guard fails (a concurrent writer or rebuild advanced - the row), we refresh from the database and check for remaining work. + invalidation state, pipeline version, and cursor position haven't + changed since we read it. If the guard fails (a concurrent writer or + rebuild advanced the row), we refresh from the database and check for + remaining work. When *persist* is False, only the in-memory object is updated — the caller is responsible for persisting the result (e.g. via @@ -146,13 +146,12 @@ def _process_batch( return len(entries) == batch_size updated = GroupDerivedData.objects.filter( - Q(id=derived.id, generation_id=derived.generation_id) + Q(id=derived.id, invalidated_at=derived.invalidated_at) & (Q(cursor_date__lt=last_date) | Q(cursor_date=last_date, cursor_id__lte=last_id)) & Q(pipeline_hash=derived.pipeline_hash) ).update(cursor_date=last_date, cursor_id=last_id, **state_update) if updated: - # Features updated in this batch (not total; a feature appears at most once per batch) for f in result.updated: metrics.incr( "issues.derived.feature_updated", sample_rate=1.0, tags={"feature": f.name} @@ -183,9 +182,6 @@ def _process_batch( "db_cursor_id": derived.cursor_id, }, ) - # A concurrent caller advanced the cursor past us. Check whether - # there are still entries beyond the refreshed cursor so we don't - # silently stop processing. return bool(_entries_after_cursor(group_id, derived.cursor_date, derived.cursor_id, 1)) @@ -298,27 +294,15 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) has_more = _process_batch(pipeline, derived, INLINE_BATCH_SIZE) if has_more: - # Derived data will be stale for any code running between now and - # when the task completes. metrics.incr("issues.derived.inline_fallback_to_async") process_group_log_task.delay(group_id) # --------------------------------------------------------------------------- -# Rebuild lifecycle: build in memory, upsert live, cache partial progress +# Rebuild lifecycle: build in memory, upsert, cache partial progress # --------------------------------------------------------------------------- -def _current_generation_id(group_id: int) -> int: - """Return the current log generation for a group. - - This is ``max(id)`` from the action log, which directly encodes the - log state at the moment a rebuild starts. - """ - result = GroupActionLogEntry.objects.filter(group_id=group_id).aggregate(Max("id")) - return result["id__max"] or 0 - - def _save_rebuild_state(rebuild_id: RebuildId, derived: GroupDerivedData) -> None: """Persist in-progress rebuild state to cache for later resumption.""" _rebuild_cache.set(rebuild_id, derived) @@ -331,80 +315,110 @@ def _load_rebuild_state(rebuild_id: RebuildId) -> GroupDerivedData | None: class PromotionResult(enum.Enum): PROMOTED = "promoted" - SUPERSEDED = "superseded" # row has a newer generation_id - CURSOR_BEHIND = "cursor_behind" # same generation, cursor is ahead + SUPERSEDED = "superseded" # a newer invalidation arrived; our work is stale + CURSOR_BEHIND = "cursor_behind" # same invalidation, but cursor is more advanced -def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: +def promote_to_live( + candidate: GroupDerivedData, + invalidated_at: datetime, +) -> PromotionResult: """Upsert the candidate's state into the row for its group. - If a row exists and the candidate's generation and cursor are at - or ahead, the row is updated in place. If no row exists, one is created. + Clears ``invalidated_at`` atomically on success via a CAS: the UPDATE + only matches if ``invalidated_at`` still equals the value we observed + when the rebuild started. If a newer invalidation arrived, the CAS + fails and the row stays flagged. - Returns SUPERSEDED when the row has a higher generation_id (a - newer rebuild already won). Returns CURSOR_BEHIND when the generation - matches but the row's cursor is more advanced. + Returns SUPERSEDED if ``invalidated_at`` changed (newer invalidation). + Returns CURSOR_BEHIND if the cursor guard failed within the same + invalidation. - The candidate object itself is not modified or persisted — it may be an - unsaved in-memory instance used only to carry the computed state. + The candidate object itself is not modified or persisted. """ values = {f: getattr(candidate, f) for f in _STATE_FIELDS} - # Try updating the existing row. The guard requires that the - # candidate's generation_id is >= the row's (so a stale rebuild - # can't overwrite a newer one), and within the same generation the - # cursor must be at or ahead. + # CAS: clear invalidated_at only if it still matches what we observed. updated = ( GroupDerivedData.objects.filter( group_id=candidate.group_id, + invalidated_at=invalidated_at, ) .filter( - Q(generation_id__lt=candidate.generation_id) - | Q( - generation_id=candidate.generation_id, - cursor_date__lt=candidate.cursor_date, - ) - | Q( - generation_id=candidate.generation_id, - cursor_date=candidate.cursor_date, - cursor_id__lte=candidate.cursor_id, - ) + Q(cursor_date__lt=candidate.cursor_date) + | Q(cursor_date=candidate.cursor_date, cursor_id__lte=candidate.cursor_id) ) - .update(**values) + .update(invalidated_at=None, **values) ) if updated: return PromotionResult.PROMOTED - # No rows updated — either the row's generation/cursor is ahead, - # or no row exists yet. Try inserting; IntegrityError means a - # row exists whose generation/cursor we didn't beat. The - # savepoint ensures the IntegrityError doesn't poison the outer - # transaction, allowing the follow-up query to run. - try: - with transaction.atomic(using=router.db_for_write(GroupDerivedData)): - GroupDerivedData.objects.create( - group_id=candidate.group_id, - **values, - ) - except IntegrityError: - # Distinguish: is the row from a newer generation, or same - # generation with a more advanced cursor? - live_gen = ( - GroupDerivedData.objects.filter(group_id=candidate.group_id) - .values_list("generation_id", flat=True) - .first() - ) - if live_gen is not None and live_gen > candidate.generation_id: - return PromotionResult.SUPERSEDED - return PromotionResult.CURSOR_BEHIND + # Check why we failed: row missing, invalidated_at changed, or cursor? + row = ( + GroupDerivedData.objects.filter(group_id=candidate.group_id) + .values_list("id", "invalidated_at") + .first() + ) + + if row is None: + # Row was deleted between our read and the CAS — the rebuild + # that deleted it will handle recreation. + return PromotionResult.SUPERSEDED - return PromotionResult.PROMOTED + _row_id, current_invalidated_at = row + if current_invalidated_at != invalidated_at: + return PromotionResult.SUPERSEDED + return PromotionResult.CURSOR_BEHIND MAX_PROMOTION_ATTEMPTS = 5 +def _build_and_insert( + group_id: int, + batch_size: int, + rebuild_id: RebuildId | None, + time_limit: timedelta, +) -> None: + """Build derived data from scratch and INSERT a new row. + + Used after a hard-delete invalidation where no row exists. No CAS + is needed — if a concurrent writer creates the row first, the INSERT + fails harmlessly. + """ + pipeline_hash = PIPELINE.pipeline_hash + current_rebuild_id = RebuildId(group_id, "deleted", pipeline_hash) + + derived: GroupDerivedData | None = None + if rebuild_id is not None and rebuild_id == current_rebuild_id: + derived = _load_rebuild_state(rebuild_id) + + if derived is None: + derived = GroupDerivedData( + group_id=group_id, + cursor_date=EPOCH, + cursor_id=0, + data={}, + pipeline_hash=pipeline_hash, + ) + + drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) + if not drained: + _save_rebuild_state(current_rebuild_id, derived) + raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) + + values = {f: getattr(derived, f) for f in _STATE_FIELDS} + try: + with transaction.atomic(using=router.db_for_write(GroupDerivedData)): + GroupDerivedData.objects.create(group_id=group_id, **values) + except IntegrityError: + # A concurrent writer already created the row — that's fine. + logger.info("issues.derived.insert_race_lost", extra={"group_id": group_id}) + + _rebuild_cache.delete(current_rebuild_id) + + def build_and_promote_derived_data( group_id: int, batch_size: int = DEFAULT_BATCH_SIZE, @@ -415,22 +429,42 @@ def build_and_promote_derived_data( The common path works entirely in memory: a transient GroupDerivedData is populated by draining the action log, then its state is upserted - into the row (2-3 queries total for single-batch groups). + into the row via a CAS on ``invalidated_at``. When *rebuild_id* is provided, previously cached partial progress is - loaded and resumed. This happens after a prior run timed out and - saved its state to cache. + loaded and resumed. - If promotion fails because the row's cursor is ahead (it received - incremental updates while we were building), we drain additional entries - to catch up and retry, bounded by MAX_PROMOTION_ATTEMPTS. + The rebuild can bail early between batches if it detects that + ``invalidated_at`` changed on the row (a newer invalidation arrived), + avoiding wasted work. Raises GroupLogTimeout (with ``rebuild_id`` set) if the time-limited drain could not finish, so the caller can re-enqueue. """ + # Determine which invalidation we're serving (if any). + # Single query: None means no row; (id, None) means row not invalidated. + row = ( + GroupDerivedData.objects.filter(group_id=group_id) + .values_list("id", "invalidated_at") + .first() + ) + if row is not None: + _row_id, invalidated_at = row + if invalidated_at is None: + # Row exists but is not invalidated — nothing to rebuild. + return + else: + # Row was hard-deleted — rebuild from scratch via INSERT. + return _build_and_insert(group_id, batch_size, rebuild_id, time_limit) + + pipeline_hash = PIPELINE.pipeline_hash + current_rebuild_id = RebuildId(group_id, invalidated_at.isoformat(), pipeline_hash) + + # Try to resume from cache. derived: GroupDerivedData | None = None if rebuild_id is not None: - derived = _load_rebuild_state(rebuild_id) + if rebuild_id == current_rebuild_id: + derived = _load_rebuild_state(rebuild_id) if derived is None: logger.info( "issues.derived.build_and_promote.cache_miss", @@ -438,34 +472,23 @@ def build_and_promote_derived_data( ) if derived is None: - # Capture the current log generation before we start draining. - # This lets promote_to_live reject stale rebuilds that started - # before a later log mutation triggered a newer rebuild. - generation_id = _current_generation_id(group_id) - pipeline_hash = PIPELINE.pipeline_hash - rebuild_id = RebuildId(group_id, generation_id, pipeline_hash) - - # In-memory only — cached on timeout for resumption. derived = GroupDerivedData( group_id=group_id, - generation_id=generation_id, cursor_date=EPOCH, cursor_id=0, data={}, pipeline_hash=pipeline_hash, ) - else: - rebuild_id = RebuildId(group_id, derived.generation_id, derived.pipeline_hash or "") result = PromotionResult.CURSOR_BEHIND attempt = 0 for attempt in range(MAX_PROMOTION_ATTEMPTS): drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - _save_rebuild_state(rebuild_id, derived) - raise GroupLogTimeout(group_id, rebuild_id=rebuild_id) + _save_rebuild_state(current_rebuild_id, derived) + raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) - result = promote_to_live(derived) + result = promote_to_live(derived, invalidated_at) if result is PromotionResult.PROMOTED: logger.info( "issues.derived.promoted", @@ -476,14 +499,14 @@ def build_and_promote_derived_data( "attempts": attempt + 1, }, ) - _rebuild_cache.delete(rebuild_id) + _rebuild_cache.delete(current_rebuild_id) return if result is PromotionResult.SUPERSEDED: - # A newer rebuild already won — retrying is futile. + # A newer invalidation already won — retrying is futile. break - _rebuild_cache.delete(rebuild_id) + _rebuild_cache.delete(current_rebuild_id) if result is PromotionResult.CURSOR_BEHIND: metrics.incr("issues.derived.promotion_exhausted", sample_rate=1.0) @@ -513,41 +536,49 @@ def invalidate_group_derived_data( group_id: int, cursor: tuple[datetime, int] | None = None, *, - hard_delete: bool = True, + hard_delete: bool = False, ) -> None: """Invalidate derived state so it is rebuilt. *hard_delete* controls the strategy: - - ``True`` (default): delete the row immediately and kick off an - async task to rebuild from scratch. Use this when the existing data is - known to be wrong and must not be served. - - ``False``: leave the current row in place and kick off a background - build-and-promote. The existing row continues serving reads until - the replacement is ready. + - ``False`` (default): flag the row with ``invalidated_at`` and kick + off a background rebuild. The existing row continues serving reads + until the rebuild completes and clears the flag. + - ``True``: delete the row immediately and kick off an async task to + rebuild from scratch. Use this when the existing data is known to + be wrong and must not be served. If *cursor* is ``(date_added, id)`` of the earliest affected entry, the invalidation only fires when the row's cursor is at or past that point; otherwise the mutation is still ahead of processing and no - invalidation is needed. *cursor* is only meaningful with - ``hard_delete=True``. + invalidation is needed. """ - if not hard_delete: - rebuild_group_derived_data.delay(group_id) - return + now = timezone.now() if cursor is None: - GroupDerivedData.objects.filter(group_id=group_id).delete() + # Unconditional invalidation — always enqueue the rebuild. + if hard_delete: + GroupDerivedData.objects.filter(group_id=group_id).delete() + else: + GroupDerivedData.objects.filter(group_id=group_id).update(invalidated_at=now) rebuild_group_derived_data.delay(group_id) return - # Only invalidate if the row has already processed past the affected point. + # Cursor-guarded: only invalidate if the row has processed past + # the affected log entry. cursor_date, cursor_id = cursor - deleted, _ = GroupDerivedData.objects.filter( + qs = GroupDerivedData.objects.filter( Q(group_id=group_id) - & (Q(cursor_date__gt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id)), - ).delete() - if deleted: + & (Q(cursor_date__gt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id)) + ) + + if hard_delete: + affected, _ = qs.delete() + else: + affected = qs.update(invalidated_at=now) + + if affected: logger.info( "issues.derived.invalidated", extra={ diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 00f4d8cc2c46..36948e70ffbe 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -45,7 +45,7 @@ def process_group_log_task(group_id: int, **kwargs: object) -> None: ) def rebuild_group_derived_data( group_id: int, - resume_generation_id: int | None = None, + resume_invalidated_at: str | None = None, resume_pipeline_hash: str | None = None, prior_runs: int = 0, **kwargs: object, @@ -74,8 +74,8 @@ def rebuild_group_derived_data( return rebuild_id: RebuildId | None = None - if resume_generation_id is not None and resume_pipeline_hash is not None: - rebuild_id = RebuildId(group_id, resume_generation_id, resume_pipeline_hash) + if resume_invalidated_at is not None and resume_pipeline_hash is not None: + rebuild_id = RebuildId(group_id, resume_invalidated_at, resume_pipeline_hash) try: build_and_promote_derived_data(group_id, rebuild_id=rebuild_id) @@ -94,7 +94,7 @@ def rebuild_group_derived_data( rid = e.rebuild_id rebuild_group_derived_data.delay( group_id, - resume_generation_id=rid.generation_id if rid else None, + resume_invalidated_at=rid.invalidated_at if rid else None, resume_pipeline_hash=rid.pipeline_hash if rid else None, prior_runs=prior_runs + 1, ) @@ -227,7 +227,7 @@ def process_project_derived_data_batch( rescheduled = False for group_id in group_ids: - remaining = timedelta(seconds=timeout_seconds - (time.monotonic() - start)) + remaining = timedelta(seconds=max(0, timeout_seconds - (time.monotonic() - start))) try: process_group_log(group_id, timeout=remaining) processed += 1 @@ -416,19 +416,24 @@ def rebuild_project_derived_data_batch( rescheduled = False for group_id in group_ids: - remaining = timedelta(seconds=timeout_seconds - (time.monotonic() - start)) + remaining = timedelta(seconds=max(0, timeout_seconds - (time.monotonic() - start))) try: build_and_promote_derived_data(group_id, time_limit=remaining) + processed += 1 + except Group.DoesNotExist: + logger.info( + "rebuild_project_derived_data_batch.group_not_found", + extra={"group_id": group_id, "project_id": project_id}, + ) except GroupLogTimeout as e: - # Re-enqueue the single group with its id so the - # partially-drained row is resumed, then continue the batch. + # Re-enqueue the single group so the partially-drained + # state is resumed, then continue the batch. rid = e.rebuild_id rebuild_group_derived_data.delay( group_id, - resume_generation_id=rid.generation_id if rid else None, + resume_invalidated_at=rid.invalidated_at if rid else None, resume_pipeline_hash=rid.pipeline_hash if rid else None, ) - processed += 1 if time.monotonic() - start >= timeout_seconds: rescheduled = True diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index 49178db0f06a..ac0180396566 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -30,26 +30,30 @@ class GroupDerivedData(DefaultFieldsModel): The pipeline is deterministic: replaying the same log produces the same state. However, the log is not strictly append-only — historical entries may be inserted, which is a primary reason rebuilds are triggered. - Three guards on the row prevent stale writes: - * **generation_id** — set to ``max(GroupActionLogEntry.id)`` when a - rebuild starts, capturing the log state the rebuild observed. A write - only succeeds if its generation_id is >= the row's, so a slow - rebuild that started before a log mutation cannot overwrite results - from a newer rebuild that observed the corrected log. + * **invalidated_at** — set to ``now()`` by ``invalidate_group_derived_data`` + when the log is mutated in a way that requires a full rebuild. ``NULL`` + means the row is up-to-date. A rebuild reads ``invalidated_at`` at + start and clears it atomically on promotion via a CAS: + ``UPDATE SET invalidated_at=NULL ... WHERE invalidated_at=``. + If a newer invalidation arrived while the rebuild was running, the CAS + fails and the row stays flagged for another rebuild. - * **cursor guard** — within the same generation, a write only succeeds - if the writer's ``(cursor_date, cursor_id)`` is at or ahead of the - row's, preventing cursor regression. + * **cursor guard** — incremental writes only succeed if the writer's + ``(cursor_date, cursor_id)`` is at or ahead of the row's, preventing + cursor regression. * **pipeline_hash** — stamped at row creation, incremental writes only succeed if the pipeline version hasn't changed since the row was read. A pipeline upgrade invalidates in-flight incremental work. - Incremental processing writes per-batch with all three guards scoped - to the row's ``id``. Rebuilds accumulate state in memory and write - once via ``promote_to_live``, which uses the generation and cursor - guards. + Incremental processing writes per-batch with cursor and pipeline_hash + guards scoped to the row's ``id``. Rebuilds accumulate state in memory + and write once via ``promote_to_live``, which uses the invalidated_at + CAS and cursor guards. + + Groups needing a rebuild can be efficiently queried via + ``WHERE invalidated_at IS NOT NULL``. See ``processing.py`` for the full lifecycle. """ @@ -58,11 +62,11 @@ class GroupDerivedData(DefaultFieldsModel): group = FlexibleForeignKey("sentry.Group", unique=True) - # Identifies the log state this row was built from. Set to - # ``max(GroupActionLogEntry.id)`` at the start of a rebuild so that - # the promote guard can reject stale rebuilds that started before a - # later log mutation triggered a newer rebuild. - generation_id = BoundedBigIntegerField(db_default=0, default=0) + # NULL means the row is up-to-date. Non-NULL means a rebuild is needed; + # the timestamp identifies *which* invalidation request the rebuild must + # satisfy. Set by invalidate_group_derived_data, cleared atomically by + # promote_to_live. + invalidated_at = models.DateTimeField(null=True, default=None) cursor_date = models.DateTimeField(default=EPOCH) cursor_id = BoundedBigIntegerField(default=0) @@ -92,6 +96,11 @@ class Meta: indexes = [ models.Index(fields=["progress", "group"]), models.Index(fields=["last_progressed_at", "group"]), + models.Index( + fields=["invalidated_at"], + condition=models.Q(invalidated_at__isnull=False), + name="sentry_gdd_needs_rebuild", + ), ] - __repr__ = sane_repr("group_id", "generation_id", "cursor_date", "cursor_id") + __repr__ = sane_repr("group_id", "invalidated_at", "cursor_date", "cursor_id") diff --git a/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py b/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py deleted file mode 100644 index 7b2e6689f98d..000000000000 --- a/src/sentry/migrations/1141_add_generation_id_to_group_derived_data.py +++ /dev/null @@ -1,22 +0,0 @@ -# Generated by Django 5.2.14 on 2026-07-09 15:43 - -import sentry.db.models.fields.bounded -from django.db import migrations - -from sentry.new_migrations.migrations import CheckedMigration - - -class Migration(CheckedMigration): - is_post_deployment = False - - dependencies = [ - ("sentry", "1140_organizationcontributors_unique_provider_hostname"), - ] - - operations = [ - migrations.AddField( - model_name="groupderiveddata", - name="generation_id", - field=sentry.db.models.fields.bounded.BoundedBigIntegerField(db_default=0, default=0), - ), - ] diff --git a/src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py b/src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py new file mode 100644 index 000000000000..864c35db46bf --- /dev/null +++ b/src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py @@ -0,0 +1,29 @@ +# Generated by Django 5.2.14 + +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + is_post_deployment = False + + dependencies = [ + ("sentry", "1140_organizationcontributors_unique_provider_hostname"), + ] + + operations = [ + migrations.AddField( + model_name="groupderiveddata", + name="invalidated_at", + field=models.DateTimeField(null=True, default=None), + ), + migrations.AddIndex( + model_name="groupderiveddata", + index=models.Index( + condition=models.Q(("invalidated_at__isnull", False)), + fields=["invalidated_at"], + name="sentry_gdd_needs_rebuild", + ), + ), + ] diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index 0c3e582c5b2d..87175d1c089a 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -46,7 +46,6 @@ PIPELINE, GroupLogTimeout, PromotionResult, - _current_generation_id, _entries_after_cursor, build_and_promote_derived_data, invalidate_group_derived_data, @@ -289,22 +288,24 @@ def test_status_toggle(self) -> None: # --- invalidation --- - def test_invalidate_deletes_live_row(self) -> None: + def test_hard_invalidate_deletes_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) process_group_log(group.id) assert GroupDerivedData.objects.filter(group_id=group.id).exists() - invalidate_group_derived_data(group.id) + invalidate_group_derived_data(group.id, hard_delete=True) assert not GroupDerivedData.objects.filter(group_id=group.id).exists() - def test_invalidate_with_cursor_deletes_if_past(self) -> None: + def test_hard_invalidate_with_cursor_deletes_if_past(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) assert derived is not None - invalidate_group_derived_data(group.id, cursor=(derived.cursor_date, derived.cursor_id)) + invalidate_group_derived_data( + group.id, cursor=(derived.cursor_date, derived.cursor_id), hard_delete=True + ) assert not GroupDerivedData.objects.filter(group_id=group.id).exists() def test_invalidate_with_cursor_noop_if_not_reached(self) -> None: @@ -328,12 +329,12 @@ def test_invalidate_then_reprocess(self) -> None: assert derived is not None assert derived.view_count == 2 - invalidate_group_derived_data(group.id) + invalidate_group_derived_data(group.id, hard_delete=True) derived = process_group_log(group.id) assert derived is not None assert derived.view_count == 2 # rebuilt from scratch - def test_invalidate_soft_schedules_rebuild(self) -> None: + def test_invalidate_soft_flags_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) @@ -344,9 +345,10 @@ def test_invalidate_soft_schedules_rebuild(self) -> None: invalidate_group_derived_data(group.id, hard_delete=False) mock_task.delay.assert_called_once_with(group.id) - # Live row is still in place + # Row is still in place but flagged for rebuild. derived.refresh_from_db() assert derived.id == original_id + assert derived.invalidated_at is not None def test_resolved_in_pull_request_proposes_fix(self) -> None: group = self.create_group() @@ -425,7 +427,7 @@ def test_pull_request_close_invalidate_and_replay_matches(self) -> None: first_progress = first.progress first_last_progressed_at = first.last_progressed_at - invalidate_group_derived_data(group.id) + invalidate_group_derived_data(group.id, hard_delete=True) second = process_group_log(group.id) assert second is not None @@ -471,7 +473,9 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: assert derived.cursor_id == first_cursor assert derived.pipeline_hash == "reset" - def test_generation_id_change_skips_incremental_write(self) -> None: + def test_invalidated_at_change_skips_incremental_write(self) -> None: + from django.utils import timezone + group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -489,22 +493,22 @@ def test_generation_id_change_skips_incremental_write(self) -> None: data={}, ) - # Simulate a rebuild promoting a new generation between our read - # and the UPDATE in _process_batch. - GroupDerivedData.objects.filter(id=derived.id).update(generation_id=999999) + # Simulate an invalidation between our read and the UPDATE in + # _process_batch — the row now has invalidated_at set. + GroupDerivedData.objects.filter(id=derived.id).update(invalidated_at=timezone.now()) processing._process_batch(processing.PIPELINE, derived, 1) derived.refresh_from_db() assert derived.cursor_id == first_cursor - assert derived.generation_id == 999999 + assert derived.invalidated_at is not None def test_invalidate_and_reprocess_restores_pipeline_hash(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) process_group_log(group.id) - invalidate_group_derived_data(group.id) + invalidate_group_derived_data(group.id, hard_delete=True) derived = process_group_log(group.id) assert derived is not None assert derived.pipeline_hash == PIPELINE.pipeline_hash @@ -525,27 +529,33 @@ def test_noop_when_on_demand_disabled(self) -> None: assert not GroupDerivedData.objects.filter(group_id=group.id).exists() -# --- Non-live row lifecycle --- +# --- Rebuild lifecycle --- @with_feature("projects:issue-action-log-write-to-db") class PromoteToLiveTest(TestCase): - def test_promote_inserts_when_no_live_row(self) -> None: + def _soft_invalidate(self, group: Group) -> datetime: + """Soft-invalidate the GDD row and return the invalidated_at value.""" + invalidate_group_derived_data(group.id, hard_delete=False) + row = GroupDerivedData.objects.get(group_id=group.id) + assert row.invalidated_at is not None + return row.invalidated_at + + def test_hard_delete_rebuild_inserts_new_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - gen = _current_generation_id(group.id) - candidate = GroupDerivedData( - group_id=group.id, generation_id=gen, cursor_date=EPOCH, cursor_id=0, data={} - ) - processing._drain_log(candidate, persist=False) - assert promote_to_live(candidate) is PromotionResult.PROMOTED + # Hard-delete invalidation removes the row. + invalidate_group_derived_data(group.id, hard_delete=True) + assert not GroupDerivedData.objects.filter(group_id=group.id).exists() + # Rebuild creates a new row via INSERT. + build_and_promote_derived_data(group.id) live = GroupDerivedData.objects.get(group_id=group.id) assert live.view_count == 1 - assert live.generation_id == gen + assert live.invalidated_at is None - def test_promote_updates_existing_live_row(self) -> None: + def test_promote_updates_existing_row(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) @@ -554,77 +564,67 @@ def test_promote_updates_existing_live_row(self) -> None: old_id = old.id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + inv_at = self._soft_invalidate(group) - gen = _current_generation_id(group.id) - candidate = GroupDerivedData( - group_id=group.id, generation_id=gen, cursor_date=EPOCH, cursor_id=0, data={} - ) + candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) processing._drain_log(candidate, persist=False) - assert promote_to_live(candidate) is PromotionResult.PROMOTED + assert promote_to_live(candidate, invalidated_at=inv_at) is PromotionResult.PROMOTED - # The existing live row was updated in place, not replaced. live = GroupDerivedData.objects.get(group_id=group.id) assert live.id == old_id assert live.view_count == 2 + assert live.invalidated_at is None # cleared by promotion def test_promote_rejected_if_cursor_behind(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - # Create a live row that has processed everything live = process_group_log(group.id) assert live is not None + inv_at = self._soft_invalidate(group) - # Build a candidate with the same generation as the live row - # but only process the first entry so the cursor is behind. - candidate = GroupDerivedData( - group_id=group.id, - generation_id=live.generation_id, - cursor_date=EPOCH, - cursor_id=0, - data={}, - ) + # Candidate only processes the first entry. + candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) processing._process_batch(PIPELINE, candidate, batch_size=1, persist=False) assert (candidate.cursor_date, candidate.cursor_id) < ( live.cursor_date, live.cursor_id, ) - assert promote_to_live(candidate) is PromotionResult.CURSOR_BEHIND + assert promote_to_live(candidate, invalidated_at=inv_at) is PromotionResult.CURSOR_BEHIND + + def test_promote_superseded_by_newer_invalidation(self) -> None: + from django.utils import timezone - def test_promote_rejected_if_stale_generation(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) - # Capture generation before more entries arrive - stale_gen = _current_generation_id(group.id) + old_inv_at = self._soft_invalidate(group) - # Build and promote with current generation - _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - build_and_promote_derived_data(group.id) - live = GroupDerivedData.objects.get(group_id=group.id) - assert live.generation_id > stale_gen + # A second invalidation arrives — the row now has a newer timestamp. + GroupDerivedData.objects.filter(group_id=group.id).update(invalidated_at=timezone.now()) - # A stale rebuild with the old generation can't overwrite even if - # it has the same cursor position. - stale_candidate = GroupDerivedData( - group_id=group.id, generation_id=stale_gen, cursor_date=EPOCH, cursor_id=0, data={} - ) - processing._drain_log(stale_candidate, persist=False) - assert promote_to_live(stale_candidate) is PromotionResult.SUPERSEDED + candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) + processing._drain_log(candidate, persist=False) + assert promote_to_live(candidate, invalidated_at=old_inv_at) is PromotionResult.SUPERSEDED def test_build_and_promote(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) + invalidate_group_derived_data(group.id, hard_delete=False) build_and_promote_derived_data(group.id) + derived = GroupDerivedData.objects.get(group_id=group.id) assert derived.view_count == 1 assert derived.data["status"] == "closed" + assert derived.invalidated_at is None - def test_build_and_promote_updates_stale_live(self) -> None: + def test_build_and_promote_updates_existing_row(self) -> None: group = self.create_group() user = self.user @@ -634,34 +634,34 @@ def test_build_and_promote_updates_stale_live(self) -> None: old_id = old.id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) + invalidate_group_derived_data(group.id, hard_delete=False) build_and_promote_derived_data(group.id) + live = GroupDerivedData.objects.get(group_id=group.id) - # Upsert updates the existing live row in place. assert live.id == old_id assert live.view_count == 2 - def test_build_and_promote_no_non_live_rows_on_success(self) -> None: + def test_build_and_promote_noop_when_not_invalidated(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) + # No invalidation — build_and_promote should be a no-op. build_and_promote_derived_data(group.id) - assert GroupDerivedData.objects.filter(group_id=group.id).exists() + derived = GroupDerivedData.objects.get(group_id=group.id) + assert derived.invalidated_at is None def test_drain_log_respects_time_limit(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - candidate = GroupDerivedData( - group_id=group.id, generation_id=0, cursor_date=EPOCH, cursor_id=0, data={} - ) + candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) - # Zero time limit forces bail after first batch drained = processing._drain_log( candidate, batch_size=2, time_limit=timedelta(0), persist=False ) assert not drained - # Processed one batch (2 entries) but not all 5 assert candidate.cursor_id > 0 entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) assert candidate.cursor_id < entries[-1].id @@ -679,25 +679,12 @@ def test_process_group_log_reenqueues_on_partial_drain(self) -> None: assert derived is not None mock_task.delay.assert_called_once_with(group.id) - def test_build_and_promote_exhaustion_leaves_live_row(self) -> None: - group = self.create_group() - _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - - process_group_log(group.id) - - with patch( - "sentry.issues.derived.processing.promote_to_live", - return_value=PromotionResult.CURSOR_BEHIND, - ): - build_and_promote_derived_data(group.id) - - # The original row is untouched. - assert GroupDerivedData.objects.filter(group_id=group.id).exists() - def test_build_and_promote_caches_on_timeout_for_resumption(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) + invalidate_group_derived_data(group.id, hard_delete=False) with patch("sentry.issues.derived.processing._drain_log", return_value=False): with pytest.raises(GroupLogTimeout) as exc_info: @@ -706,10 +693,11 @@ def test_build_and_promote_caches_on_timeout_for_resumption(self) -> None: assert exc_info.value.group_id == group.id assert exc_info.value.rebuild_id is not None - # Resuming with that cache_key completes the promotion. + # Resuming completes the promotion. build_and_promote_derived_data(group.id, rebuild_id=exc_info.value.rebuild_id) promoted = GroupDerivedData.objects.get(group_id=group.id) assert promoted.view_count == 5 + assert promoted.invalidated_at is None def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: from sentry.issues.derived.processing import _load_rebuild_state @@ -717,8 +705,9 @@ def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + process_group_log(group.id) + invalidate_group_derived_data(group.id, hard_delete=False) - # First run: drain 2 entries then timeout. with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data(group.id, batch_size=2, time_limit=timedelta(0)) @@ -729,7 +718,6 @@ def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: first_cursor = state.cursor_id assert first_cursor > 0 - # Second run resumes and times out again — cursor must advance. with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data( group.id, rebuild_id=rid, batch_size=2, time_limit=timedelta(0) @@ -952,7 +940,7 @@ def test_round_trip_preserves_state(self) -> None: first_progress = first.progress first_last_progressed_at = first.last_progressed_at - invalidate_group_derived_data(group.id) + invalidate_group_derived_data(group.id, hard_delete=True) second = process_group_log(group.id) assert second is not None diff --git a/tests/sentry/tasks/test_merge.py b/tests/sentry/tasks/test_merge.py index b03c8348b322..fa5032f5a43e 100644 --- a/tests/sentry/tasks/test_merge.py +++ b/tests/sentry/tasks/test_merge.py @@ -3,7 +3,7 @@ from unittest.mock import patch from sentry import buffer, eventstream -from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData +from sentry.issues.models.groupderiveddata import GroupDerivedData from sentry.models.group import Group from sentry.models.groupenvironment import GroupEnvironment from sentry.models.groupmeta import GroupMeta @@ -286,12 +286,10 @@ def test_merge_invalidates_derived_data(self) -> None: assert Group.objects.filter(id=old_group.id).exists() is False - # The derived data is invalidated: the stale row is deleted and rebuilt - # from scratch by the scheduled processing task. - rebuilt = GroupDerivedData.objects.get(group_id=new_group.id) - assert rebuilt.id != derived.id - assert rebuilt.cursor_date == EPOCH - assert rebuilt.cursor_id == 0 + # The derived data is soft-invalidated: the row is flagged for + # rebuild but continues serving reads until the rebuild completes. + derived.refresh_from_db() + assert derived.invalidated_at is not None @mock_redis_buffer() def test_merge_original_group_id(self) -> None: From 506ee79d8d1691f4fb1eef6fb81d0db189723b66 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Mon, 20 Jul 2026 11:33:01 -0700 Subject: [PATCH 8/9] cleanup --- src/sentry/issues/derived/processing.py | 26 ++++++------------- src/sentry/issues/derived/tasks.py | 25 ++++++++++++------ .../sentry/issues/derived/test_processing.py | 6 ++--- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index ac6872f94a39..0be7ca7b06ee 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -40,13 +40,13 @@ class RebuildId(NamedTuple): """Uniquely identifies a rebuild attempt for a group.""" group_id: int - invalidated_at: str # ISO timestamp or "deleted" for hard-delete rebuilds + invalidated_at: datetime | None # None for hard-delete rebuilds pipeline_hash: str # Cache for in-progress rebuild state. _rebuild_cache = CacheMapping[RebuildId, GroupDerivedData]( - lambda k: f"{k.group_id}:{k.invalidated_at}:{k.pipeline_hash}", + lambda k: f"{k.group_id}:{k.invalidated_at.isoformat() if k.invalidated_at else 'deleted'}:{k.pipeline_hash}", namespace="gdd-rebuild", ttl_seconds=86400, ) @@ -303,16 +303,6 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) # --------------------------------------------------------------------------- -def _save_rebuild_state(rebuild_id: RebuildId, derived: GroupDerivedData) -> None: - """Persist in-progress rebuild state to cache for later resumption.""" - _rebuild_cache.set(rebuild_id, derived) - - -def _load_rebuild_state(rebuild_id: RebuildId) -> GroupDerivedData | None: - """Load in-progress rebuild state from cache into an unsaved instance.""" - return _rebuild_cache.get(rebuild_id) - - class PromotionResult(enum.Enum): PROMOTED = "promoted" SUPERSEDED = "superseded" # a newer invalidation arrived; our work is stale @@ -388,11 +378,11 @@ def _build_and_insert( fails harmlessly. """ pipeline_hash = PIPELINE.pipeline_hash - current_rebuild_id = RebuildId(group_id, "deleted", pipeline_hash) + current_rebuild_id = RebuildId(group_id, None, pipeline_hash) derived: GroupDerivedData | None = None if rebuild_id is not None and rebuild_id == current_rebuild_id: - derived = _load_rebuild_state(rebuild_id) + derived = _rebuild_cache.get(rebuild_id) if derived is None: derived = GroupDerivedData( @@ -405,7 +395,7 @@ def _build_and_insert( drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - _save_rebuild_state(current_rebuild_id, derived) + _rebuild_cache.set(current_rebuild_id, derived) raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) values = {f: getattr(derived, f) for f in _STATE_FIELDS} @@ -458,13 +448,13 @@ def build_and_promote_derived_data( return _build_and_insert(group_id, batch_size, rebuild_id, time_limit) pipeline_hash = PIPELINE.pipeline_hash - current_rebuild_id = RebuildId(group_id, invalidated_at.isoformat(), pipeline_hash) + current_rebuild_id = RebuildId(group_id, invalidated_at, pipeline_hash) # Try to resume from cache. derived: GroupDerivedData | None = None if rebuild_id is not None: if rebuild_id == current_rebuild_id: - derived = _load_rebuild_state(rebuild_id) + derived = _rebuild_cache.get(rebuild_id) if derived is None: logger.info( "issues.derived.build_and_promote.cache_miss", @@ -485,7 +475,7 @@ def build_and_promote_derived_data( for attempt in range(MAX_PROMOTION_ATTEMPTS): drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - _save_rebuild_state(current_rebuild_id, derived) + _rebuild_cache.set(current_rebuild_id, derived) raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) result = promote_to_live(derived, invalidated_at) diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 36948e70ffbe..49eba33af1f3 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -75,7 +75,13 @@ def rebuild_group_derived_data( rebuild_id: RebuildId | None = None if resume_invalidated_at is not None and resume_pipeline_hash is not None: - rebuild_id = RebuildId(group_id, resume_invalidated_at, resume_pipeline_hash) + from datetime import datetime, timezone + + rebuild_id = RebuildId( + group_id, + datetime.fromisoformat(resume_invalidated_at).replace(tzinfo=timezone.utc), + resume_pipeline_hash, + ) try: build_and_promote_derived_data(group_id, rebuild_id=rebuild_id) @@ -94,7 +100,9 @@ def rebuild_group_derived_data( rid = e.rebuild_id rebuild_group_derived_data.delay( group_id, - resume_invalidated_at=rid.invalidated_at if rid else None, + resume_invalidated_at=rid.invalidated_at.isoformat() + if rid and rid.invalidated_at + else None, resume_pipeline_hash=rid.pipeline_hash if rid else None, prior_runs=prior_runs + 1, ) @@ -298,12 +306,11 @@ def process_project_derived_data_batch( silo_mode=SiloMode.CELL, ) def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: - """Rebuild derived data for all groups in a project via build-and-promote. + """Rebuild derived data for every group in a project. - Unlike process_project_derived_data (which only targets groups missing a - GDD row), this task covers every group in the project — creating derived - data where missing and replacing existing live rows with freshly built ones. - Existing live rows continue serving reads until each replacement is promoted. + Partitions groups into ID ranges and fans out a batch task for each + range. Each batch calls ``build_and_promote_derived_data`` per group, + which replaces existing rows via CAS while they continue serving reads. """ from sentry import options from sentry.models.group import Group @@ -431,7 +438,9 @@ def rebuild_project_derived_data_batch( rid = e.rebuild_id rebuild_group_derived_data.delay( group_id, - resume_invalidated_at=rid.invalidated_at if rid else None, + resume_invalidated_at=rid.invalidated_at.isoformat() + if rid and rid.invalidated_at + else None, resume_pipeline_hash=rid.pipeline_hash if rid else None, ) diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index 87175d1c089a..ea3b7a010be7 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -700,7 +700,7 @@ def test_build_and_promote_caches_on_timeout_for_resumption(self) -> None: assert promoted.invalidated_at is None def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: - from sentry.issues.derived.processing import _load_rebuild_state + from sentry.issues.derived.processing import _rebuild_cache group = self.create_group() for _ in range(5): @@ -713,7 +713,7 @@ def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: rid = exc_info.value.rebuild_id assert rid is not None - state = _load_rebuild_state(rid) + state = _rebuild_cache.get(rid) assert state is not None first_cursor = state.cursor_id assert first_cursor > 0 @@ -725,7 +725,7 @@ def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: rid2 = exc_info.value.rebuild_id assert rid2 is not None - state = _load_rebuild_state(rid2) + state = _rebuild_cache.get(rid2) assert state is not None assert state.cursor_id > first_cursor From 4793b60134fb7fea1fca04080cd002ce4631e538 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Mon, 20 Jul 2026 15:06:22 -0700 Subject: [PATCH 9/9] back to basics --- migrations_lockfile.txt | 2 +- src/sentry/issues/derived/processing.py | 342 ++++++++---------- src/sentry/issues/derived/tasks.py | 107 +++--- src/sentry/issues/models/groupderiveddata.py | 44 +-- ...add_generated_at_to_group_derived_data.py} | 15 +- src/sentry/options/defaults.py | 10 - .../sentry/issues/derived/test_processing.py | 252 +++++++------ tests/sentry/tasks/test_merge.py | 5 +- 8 files changed, 361 insertions(+), 416 deletions(-) rename src/sentry/migrations/{1141_add_invalidated_at_to_group_derived_data.py => 1141_add_generated_at_to_group_derived_data.py} (53%) diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index b41d6ad12ae8..79ba6647f0e6 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -31,7 +31,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0027_add_seer_run_coding_agent_handoff -sentry: 1141_add_invalidated_at_to_group_derived_data +sentry: 1141_add_generated_at_to_group_derived_data social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/issues/derived/processing.py b/src/sentry/issues/derived/processing.py index 0be7ca7b06ee..418af645e1c9 100644 --- a/src/sentry/issues/derived/processing.py +++ b/src/sentry/issues/derived/processing.py @@ -9,11 +9,10 @@ from django.db.models import Q from django.utils import timezone -from sentry import options from sentry.issues.derived.aggregators import AGGREGATORS from sentry.issues.derived.framework import Pipeline from sentry.issues.derived.store import GroupDerivedDataStore -from sentry.issues.derived.tasks import process_group_log_task, rebuild_group_derived_data +from sentry.issues.derived.tasks import generate_group_derived_data, process_group_log_task from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.issues.models.groupderiveddata import EPOCH, GroupDerivedData from sentry.models.group import Group @@ -30,24 +29,24 @@ # Fields that constitute the derived state, written by promote_to_live. # Derived by excluding identity, control, and auto-managed fields from the # model — new columns are automatically included unless explicitly excluded. -_EXCLUDED_FIELDS = frozenset({"id", "group_id", "invalidated_at", "date_added", "date_updated"}) +_EXCLUDED_FIELDS = frozenset({"id", "group_id", "date_added", "date_updated"}) _STATE_FIELDS = tuple( f.attname for f in GroupDerivedData._meta.concrete_fields if f.attname not in _EXCLUDED_FIELDS ) -class RebuildId(NamedTuple): - """Uniquely identifies a rebuild attempt for a group.""" +class GenerationId(NamedTuple): + """Uniquely identifies a generation attempt for a group.""" group_id: int - invalidated_at: datetime | None # None for hard-delete rebuilds + generated_at: datetime # when this generation started; reflects log state observed pipeline_hash: str -# Cache for in-progress rebuild state. -_rebuild_cache = CacheMapping[RebuildId, GroupDerivedData]( - lambda k: f"{k.group_id}:{k.invalidated_at.isoformat() if k.invalidated_at else 'deleted'}:{k.pipeline_hash}", - namespace="gdd-rebuild", +# Cache for in-progress generation state. +_generation_cache = CacheMapping[GenerationId, GroupDerivedData]( + lambda k: f"{k.group_id}:{k.generated_at.isoformat()}:{k.pipeline_hash}", + namespace="gdd-generation", ttl_seconds=86400, ) @@ -58,12 +57,8 @@ class ProcessingStrategy(enum.Enum): INLINE = "inline" # try to process all pending actions quickly; fall back to ASYNC -def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | None: - """Get the GroupDerivedData row for a group, optionally creating one. - - When ``issues.derived-data.create-on-demand`` is enabled, a row is - created if none exists. When disabled, returns None — processing will be - a no-op until a backfill task creates and promotes a row. +def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData: + """Get or create the GroupDerivedData row for a group. Raises Group.DoesNotExist if the group has been deleted. """ @@ -72,21 +67,22 @@ def _ensure_derived(group_id: int, pipeline_hash: str) -> GroupDerivedData | Non except GroupDerivedData.DoesNotExist: pass - if not options.get("issues.derived-data.create-on-demand"): - return None - if not Group.objects.filter(id=group_id).exists(): raise Group.DoesNotExist(f"Group {group_id} does not exist") - derived, _created = GroupDerivedData.objects.get_or_create( - group_id=group_id, - defaults={ - "cursor_date": EPOCH, - "cursor_id": 0, - "data": {}, - "pipeline_hash": pipeline_hash, - }, - ) + try: + derived, _created = GroupDerivedData.objects.get_or_create( + group_id=group_id, + defaults={ + "cursor_date": EPOCH, + "cursor_id": 0, + "data": {}, + "pipeline_hash": pipeline_hash, + }, + ) + except IntegrityError: + # The group was deleted between our exists() check and the INSERT. + raise Group.DoesNotExist(f"Group {group_id} does not exist") return derived @@ -114,16 +110,23 @@ def _process_batch( Process up to `batch_size` entries for a group. Updates derived in place. Returns True if there are more entries to process. - When *persist* is True (default), the update is written to the database - with an optimistic concurrency guard that ensures the row's identity, - invalidation state, pipeline version, and cursor position haven't - changed since we read it. If the guard fails (a concurrent writer or - rebuild advanced the row), we refresh from the database and check for - remaining work. + Concurrency: multiple callers may process the same group simultaneously. + Safety relies on two properties: + + 1. The action log is append-only and the pipeline is deterministic, so + any caller processing the same entries produces the same result. + 2. The UPDATE uses a cursor guard that only succeeds if no + other caller has already advanced the cursor past our batch. If it + fails (updated == 0), a concurrent caller already wrote a superset + of our work, so we refresh and check if more remains. + + This is an optimistic concurrency scheme — no locks are held, and the + last-writer-wins semantics are safe because all writers compute the + same deterministic result for overlapping entry ranges. When *persist* is False, only the in-memory object is updated — the caller is responsible for persisting the result (e.g. via - ``promote_to_live``). Used for background rebuilds that accumulate + ``promote_to_live``). Used for full generations that accumulate state in memory and write once at the end. """ group_id = derived.group_id @@ -146,12 +149,13 @@ def _process_batch( return len(entries) == batch_size updated = GroupDerivedData.objects.filter( - Q(id=derived.id, invalidated_at=derived.invalidated_at) + Q(id=derived.id, generated_at=derived.generated_at) & (Q(cursor_date__lt=last_date) | Q(cursor_date=last_date, cursor_id__lte=last_id)) & Q(pipeline_hash=derived.pipeline_hash) ).update(cursor_date=last_date, cursor_id=last_id, **state_update) if updated: + # Features updated in this batch (not total; a feature appears at most once per batch) for f in result.updated: metrics.incr( "issues.derived.feature_updated", sample_rate=1.0, tags={"feature": f.name} @@ -182,15 +186,18 @@ def _process_batch( "db_cursor_id": derived.cursor_id, }, ) + # A concurrent caller advanced the cursor past us. Check whether + # there are still entries beyond the refreshed cursor so we don't + # silently stop processing. return bool(_entries_after_cursor(group_id, derived.cursor_date, derived.cursor_id, 1)) class GroupLogTimeout(Exception): """Raised when processing cannot finish within its time budget.""" - def __init__(self, group_id: int, rebuild_id: RebuildId | None = None) -> None: + def __init__(self, group_id: int, generation_id: GenerationId | None = None) -> None: self.group_id = group_id - self.rebuild_id = rebuild_id + self.generation_id = generation_id super().__init__(group_id) @@ -222,7 +229,7 @@ def _drain_log( # --------------------------------------------------------------------------- -# Live-row processing (incremental, on event arrival) +# Incremental processing (on event arrival) # --------------------------------------------------------------------------- @@ -231,10 +238,9 @@ def process_group_log( batch_size: int = DEFAULT_BATCH_SIZE, pipeline: Pipeline[GroupActionLogEntry] | None = None, timeout: timedelta | None = None, -) -> GroupDerivedData | None: +) -> GroupDerivedData: """Fully drain all pending entries for a group's row. - Returns None if no row exists and on-demand creation is disabled. Raises Group.DoesNotExist if the group has been deleted. Raises GroupLogTimeout if *timeout* elapses before all entries are processed. @@ -244,9 +250,6 @@ def process_group_log( with transaction.atomic(using=router.db_for_write(GroupDerivedData)): derived = _ensure_derived(group_id, p.pipeline_hash) - if derived is None: - return None - time_limit = timeout if timeout is not None else DEFAULT_TIME_LIMIT drained = _drain_log(derived, batch_size, p, time_limit=time_limit) if not drained: @@ -289,9 +292,6 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) except ObjectDoesNotExist: return - if derived is None: - return - has_more = _process_batch(pipeline, derived, INLINE_BATCH_SIZE) if has_more: metrics.incr("issues.derived.inline_fallback_to_async") @@ -299,65 +299,75 @@ def trigger_group_log_processing(group_id: int, *, strategy: ProcessingStrategy) # --------------------------------------------------------------------------- -# Rebuild lifecycle: build in memory, upsert, cache partial progress +# Generation lifecycle: build in memory, upsert, cache partial progress # --------------------------------------------------------------------------- class PromotionResult(enum.Enum): PROMOTED = "promoted" - SUPERSEDED = "superseded" # a newer invalidation arrived; our work is stale - CURSOR_BEHIND = "cursor_behind" # same invalidation, but cursor is more advanced + SUPERSEDED = "superseded" # a newer generation already promoted + CURSOR_BEHIND = "cursor_behind" # same generation, but cursor is more advanced -def promote_to_live( - candidate: GroupDerivedData, - invalidated_at: datetime, -) -> PromotionResult: +def promote_to_live(candidate: GroupDerivedData) -> PromotionResult: """Upsert the candidate's state into the row for its group. - Clears ``invalidated_at`` atomically on success via a CAS: the UPDATE - only matches if ``invalidated_at`` still equals the value we observed - when the rebuild started. If a newer invalidation arrived, the CAS - fails and the row stays flagged. + The UPDATE guard requires that ``candidate.generated_at`` is >= the + row's (newer generation wins) and the cursor is at or ahead. On + success, all state fields (including ``generated_at``) are stamped. - Returns SUPERSEDED if ``invalidated_at`` changed (newer invalidation). - Returns CURSOR_BEHIND if the cursor guard failed within the same - invalidation. + Returns SUPERSEDED if the row has a newer ``generated_at``. + Returns CURSOR_BEHIND if the cursor guard failed. - The candidate object itself is not modified or persisted. + The candidate object itself is not persisted — it may be an unsaved + in-memory instance used only to carry the computed state. """ + generated_at = candidate.generated_at values = {f: getattr(candidate, f) for f in _STATE_FIELDS} - # CAS: clear invalidated_at only if it still matches what we observed. updated = ( GroupDerivedData.objects.filter( group_id=candidate.group_id, - invalidated_at=invalidated_at, ) .filter( - Q(cursor_date__lt=candidate.cursor_date) - | Q(cursor_date=candidate.cursor_date, cursor_id__lte=candidate.cursor_id) + Q(generated_at__lt=generated_at) + | Q( + generated_at=generated_at, + cursor_date__lt=candidate.cursor_date, + ) + | Q( + generated_at=generated_at, + cursor_date=candidate.cursor_date, + cursor_id__lte=candidate.cursor_id, + ) ) - .update(invalidated_at=None, **values) + .update(**values) ) if updated: return PromotionResult.PROMOTED - # Check why we failed: row missing, invalidated_at changed, or cursor? + # Check why we failed: row missing or newer generation? row = ( GroupDerivedData.objects.filter(group_id=candidate.group_id) - .values_list("id", "invalidated_at") + .values_list("id", "generated_at") .first() ) if row is None: - # Row was deleted between our read and the CAS — the rebuild - # that deleted it will handle recreation. - return PromotionResult.SUPERSEDED + # Row doesn't exist — try to create it. + try: + with transaction.atomic(using=router.db_for_write(GroupDerivedData)): + GroupDerivedData.objects.create( + group_id=candidate.group_id, + **values, + ) + except IntegrityError: + return PromotionResult.CURSOR_BEHIND + return PromotionResult.PROMOTED - _row_id, current_invalidated_at = row - if current_invalidated_at != invalidated_at: + _row_id, current_generated_at = row + if current_generated_at > generated_at: return PromotionResult.SUPERSEDED return PromotionResult.CURSOR_BEHIND @@ -365,120 +375,61 @@ def promote_to_live( MAX_PROMOTION_ATTEMPTS = 5 -def _build_and_insert( - group_id: int, - batch_size: int, - rebuild_id: RebuildId | None, - time_limit: timedelta, -) -> None: - """Build derived data from scratch and INSERT a new row. - - Used after a hard-delete invalidation where no row exists. No CAS - is needed — if a concurrent writer creates the row first, the INSERT - fails harmlessly. - """ - pipeline_hash = PIPELINE.pipeline_hash - current_rebuild_id = RebuildId(group_id, None, pipeline_hash) - - derived: GroupDerivedData | None = None - if rebuild_id is not None and rebuild_id == current_rebuild_id: - derived = _rebuild_cache.get(rebuild_id) - - if derived is None: - derived = GroupDerivedData( - group_id=group_id, - cursor_date=EPOCH, - cursor_id=0, - data={}, - pipeline_hash=pipeline_hash, - ) - - drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) - if not drained: - _rebuild_cache.set(current_rebuild_id, derived) - raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) - - values = {f: getattr(derived, f) for f in _STATE_FIELDS} - try: - with transaction.atomic(using=router.db_for_write(GroupDerivedData)): - GroupDerivedData.objects.create(group_id=group_id, **values) - except IntegrityError: - # A concurrent writer already created the row — that's fine. - logger.info("issues.derived.insert_race_lost", extra={"group_id": group_id}) - - _rebuild_cache.delete(current_rebuild_id) - - def build_and_promote_derived_data( group_id: int, batch_size: int = DEFAULT_BATCH_SIZE, - rebuild_id: RebuildId | None = None, + generation_id: GenerationId | None = None, time_limit: timedelta = DEFAULT_TIME_LIMIT, ) -> None: """Build derived data from scratch and upsert into the row. - The common path works entirely in memory: a transient GroupDerivedData - is populated by draining the action log, then its state is upserted - into the row via a CAS on ``invalidated_at``. - - When *rebuild_id* is provided, previously cached partial progress is - loaded and resumed. + Drains the full action log into an in-memory object, then upserts + via ``promote_to_live`` with a CAS on ``generated_at``. The + generation's ``generated_at`` is captured at start — it reflects the + log state observed, not when the generation finished. - The rebuild can bail early between batches if it detects that - ``invalidated_at`` changed on the row (a newer invalidation arrived), - avoiding wasted work. + When *generation_id* is provided, previously cached partial progress + is loaded and resumed. - Raises GroupLogTimeout (with ``rebuild_id`` set) if the time-limited + Raises GroupLogTimeout (with ``generation_id`` set) if the time-limited drain could not finish, so the caller can re-enqueue. """ - # Determine which invalidation we're serving (if any). - # Single query: None means no row; (id, None) means row not invalidated. - row = ( - GroupDerivedData.objects.filter(group_id=group_id) - .values_list("id", "invalidated_at") - .first() - ) - if row is not None: - _row_id, invalidated_at = row - if invalidated_at is None: - # Row exists but is not invalidated — nothing to rebuild. - return - else: - # Row was hard-deleted — rebuild from scratch via INSERT. - return _build_and_insert(group_id, batch_size, rebuild_id, time_limit) - pipeline_hash = PIPELINE.pipeline_hash - current_rebuild_id = RebuildId(group_id, invalidated_at, pipeline_hash) + generated_at: datetime # Try to resume from cache. derived: GroupDerivedData | None = None - if rebuild_id is not None: - if rebuild_id == current_rebuild_id: - derived = _rebuild_cache.get(rebuild_id) + if generation_id is not None: + derived = _generation_cache.get(generation_id) + generated_at = generation_id.generated_at if derived is None: logger.info( "issues.derived.build_and_promote.cache_miss", - extra={"group_id": group_id, "rebuild_id": rebuild_id}, + extra={"group_id": group_id, "generation_id": generation_id}, ) if derived is None: + generated_at = timezone.now() derived = GroupDerivedData( group_id=group_id, + generated_at=generated_at, cursor_date=EPOCH, cursor_id=0, data={}, pipeline_hash=pipeline_hash, ) + current_gen_id = GenerationId(group_id, generated_at, pipeline_hash) + result = PromotionResult.CURSOR_BEHIND attempt = 0 for attempt in range(MAX_PROMOTION_ATTEMPTS): drained = _drain_log(derived, batch_size, time_limit=time_limit, persist=False) if not drained: - _rebuild_cache.set(current_rebuild_id, derived) - raise GroupLogTimeout(group_id, rebuild_id=current_rebuild_id) + _generation_cache.set(current_gen_id, derived) + raise GroupLogTimeout(group_id, generation_id=current_gen_id) - result = promote_to_live(derived, invalidated_at) + result = promote_to_live(derived) if result is PromotionResult.PROMOTED: logger.info( "issues.derived.promoted", @@ -489,14 +440,25 @@ def build_and_promote_derived_data( "attempts": attempt + 1, }, ) - _rebuild_cache.delete(current_rebuild_id) + _generation_cache.delete(current_gen_id) return if result is PromotionResult.SUPERSEDED: - # A newer invalidation already won — retrying is futile. + # A newer generation already won — retrying is futile. + break + + # CURSOR_BEHIND: the live row's cursor advanced past ours (via + # incremental processing). Load the live row's full state so the + # next _drain_log builds on the correct base, not stale aggregations. + live_row = GroupDerivedData.objects.filter(group_id=group_id).first() + if live_row is None: break + for field in _STATE_FIELDS: + setattr(derived, field, getattr(live_row, field)) + # Keep our generated_at — the live row's may be older. + derived.generated_at = generated_at - _rebuild_cache.delete(current_rebuild_id) + _generation_cache.delete(current_gen_id) if result is PromotionResult.CURSOR_BEHIND: metrics.incr("issues.derived.promotion_exhausted", sample_rate=1.0) @@ -528,53 +490,43 @@ def invalidate_group_derived_data( *, hard_delete: bool = False, ) -> None: - """Invalidate derived state so it is rebuilt. + """Invalidate derived state so it is regenerated. *hard_delete* controls the strategy: - - ``False`` (default): flag the row with ``invalidated_at`` and kick - off a background rebuild. The existing row continues serving reads - until the rebuild completes and clears the flag. - - ``True``: delete the row immediately and kick off an async task to - rebuild from scratch. Use this when the existing data is known to + - ``False`` (default): enqueue a generation task. The existing row + continues serving reads until the generation promotes and stamps + a newer ``generated_at``. + - ``True``: delete the row immediately, then enqueue a generation + task to create a new one. Use when the existing data is known to be wrong and must not be served. If *cursor* is ``(date_added, id)`` of the earliest affected entry, the invalidation only fires when the row's cursor is at or past that point; otherwise the mutation is still ahead of processing and no - invalidation is needed. + invalidation is needed. *cursor* is only meaningful with + ``hard_delete=True``. """ - now = timezone.now() - - if cursor is None: - # Unconditional invalidation — always enqueue the rebuild. - if hard_delete: + if hard_delete: + if cursor is None: GroupDerivedData.objects.filter(group_id=group_id).delete() else: - GroupDerivedData.objects.filter(group_id=group_id).update(invalidated_at=now) - rebuild_group_derived_data.delay(group_id) - return - - # Cursor-guarded: only invalidate if the row has processed past - # the affected log entry. - cursor_date, cursor_id = cursor - qs = GroupDerivedData.objects.filter( - Q(group_id=group_id) - & (Q(cursor_date__gt=cursor_date) | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id)) - ) - - if hard_delete: - affected, _ = qs.delete() - else: - affected = qs.update(invalidated_at=now) - - if affected: - logger.info( - "issues.derived.invalidated", - extra={ - "group_id": group_id, - "cursor_date": str(cursor_date), - "cursor_id": cursor_id, - }, - ) - rebuild_group_derived_data.delay(group_id) + cursor_date, cursor_id = cursor + deleted, _ = GroupDerivedData.objects.filter( + Q(group_id=group_id) + & ( + Q(cursor_date__gt=cursor_date) + | Q(cursor_date=cursor_date, cursor_id__gte=cursor_id) + ) + ).delete() + if not deleted: + return + logger.info( + "issues.derived.invalidated", + extra={ + "group_id": group_id, + "cursor_date": str(cursor_date), + "cursor_id": cursor_id, + }, + ) + generate_group_derived_data.delay(group_id) diff --git a/src/sentry/issues/derived/tasks.py b/src/sentry/issues/derived/tasks.py index 49eba33af1f3..a7a4a0327913 100644 --- a/src/sentry/issues/derived/tasks.py +++ b/src/sentry/issues/derived/tasks.py @@ -13,11 +13,11 @@ BATCH_RETRIGGER_TIMEOUT = timedelta(seconds=20) # self-reschedule before the hard kill _BATCH_TASK_KEY = "process_project_derived_data_batch" -_REBUILD_BATCH_TASK_KEY = "rebuild_project_derived_data_batch" -_REBUILD_GROUP_TASK_KEY = "rebuild_group_derived_data" +_GENERATE_BATCH_TASK_KEY = "generate_project_derived_data_batch" +_GENERATE_GROUP_TASK_KEY = "generate_group_derived_data" # Cap self-rescheduling rebuilds to avoid infinite loops on very large groups. -_MAX_REBUILD_RUNS = 20 +_MAX_GENERATION_RUNS = 20 # Hard limit on group IDs loaded per project-level task to bound memory. _MAX_PROJECT_GROUPS = 10_000 @@ -39,75 +39,78 @@ def process_group_log_task(group_id: int, **kwargs: object) -> None: @instrumented_task( - name="sentry.issues.derived.tasks.rebuild_group_derived_data", + name="sentry.issues.derived.tasks.generate_group_derived_data", namespace=issues_tasks, silo_mode=SiloMode.CELL, ) -def rebuild_group_derived_data( +def generate_group_derived_data( group_id: int, - resume_invalidated_at: str | None = None, + resume_generated_at: str | None = None, resume_pipeline_hash: str | None = None, prior_runs: int = 0, **kwargs: object, ) -> None: - """Build a new GroupDerivedData row from scratch and promote it.""" + """Generate derived data for a group by draining its action log.""" from taskbroker_client.state import current_task from sentry.issues.derived.processing import ( + GenerationId, GroupLogTimeout, - RebuildId, build_and_promote_derived_data, ) from sentry.taskworker.selfchain_idempotency import already_spawned, mark_spawned task_state = current_task() activation_id = task_state.id if task_state else None - if activation_id and already_spawned(_REBUILD_GROUP_TASK_KEY, activation_id): + if activation_id and already_spawned(_GENERATE_GROUP_TASK_KEY, activation_id): logger.info( - "rebuild_group_derived_data.duplicate_skipped", + "generate_group_derived_data.duplicate_skipped", extra={"group_id": group_id, "activation_id": activation_id}, ) metrics.incr( "taskworker.selfchain.duplicate_skipped", - tags={"task": _REBUILD_GROUP_TASK_KEY}, + tags={"task": _GENERATE_GROUP_TASK_KEY}, ) return - rebuild_id: RebuildId | None = None - if resume_invalidated_at is not None and resume_pipeline_hash is not None: + generation_id: GenerationId | None = None + if resume_generated_at is not None and resume_pipeline_hash is not None: from datetime import datetime, timezone - rebuild_id = RebuildId( + generation_id = GenerationId( group_id, - datetime.fromisoformat(resume_invalidated_at).replace(tzinfo=timezone.utc), + datetime.fromisoformat(resume_generated_at).replace(tzinfo=timezone.utc), resume_pipeline_hash, ) + from sentry.models.group import Group + try: - build_and_promote_derived_data(group_id, rebuild_id=rebuild_id) + build_and_promote_derived_data(group_id, generation_id=generation_id) + except Group.DoesNotExist: + logger.info("generate_group_derived_data.group_not_found", extra={"group_id": group_id}) + return except GroupLogTimeout as e: - if prior_runs + 1 >= _MAX_REBUILD_RUNS: + if prior_runs + 1 >= _MAX_GENERATION_RUNS: logger.error( - "rebuild_group_derived_data.max_runs_exceeded", + "generate_group_derived_data.max_runs_exceeded", extra={ "group_id": group_id, - "rebuild_id": e.rebuild_id, + "generation_id": e.generation_id, "prior_runs": prior_runs + 1, }, ) - metrics.incr("issues.derived.rebuild_max_runs_exceeded", sample_rate=1.0) + metrics.incr("issues.derived.generate_max_runs_exceeded", sample_rate=1.0) return - rid = e.rebuild_id - rebuild_group_derived_data.delay( + gen_id = e.generation_id + generate_group_derived_data.delay( group_id, - resume_invalidated_at=rid.invalidated_at.isoformat() - if rid and rid.invalidated_at - else None, - resume_pipeline_hash=rid.pipeline_hash if rid else None, + resume_generated_at=gen_id.generated_at.isoformat() if gen_id else None, + resume_pipeline_hash=gen_id.pipeline_hash if gen_id else None, prior_runs=prior_runs + 1, ) if activation_id: - mark_spawned(_REBUILD_GROUP_TASK_KEY, activation_id) + mark_spawned(_GENERATE_GROUP_TASK_KEY, activation_id) @instrumented_task( @@ -296,17 +299,17 @@ def process_project_derived_data_batch( # --------------------------------------------------------------------------- -# Project-level rebuild: build-and-promote without deleting existing live rows +# Project-level generation: build-and-promote for all groups # --------------------------------------------------------------------------- @instrumented_task( - name="sentry.issues.derived.tasks.rebuild_project_derived_data", + name="sentry.issues.derived.tasks.generate_project_derived_data", namespace=issues_tasks, silo_mode=SiloMode.CELL, ) -def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: - """Rebuild derived data for every group in a project. +def generate_project_derived_data(project_id: int, **kwargs: object) -> None: + """Generate derived data for every group in a project. Partitions groups into ID ranges and fans out a batch task for each range. Each batch calls ``build_and_promote_derived_data`` per group, @@ -330,7 +333,7 @@ def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: if len(group_ids) >= _MAX_PROJECT_GROUPS: logger.error( - "rebuild_project_derived_data.too_many_groups", + "generate_project_derived_data.too_many_groups", extra={ "project_id": project_id, "limit": _MAX_PROJECT_GROUPS, @@ -343,7 +346,7 @@ def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: if len(ranges) > max_tasks: logger.error( - "rebuild_project_derived_data.too_many_tasks", + "generate_project_derived_data.too_many_tasks", extra={ "project_id": project_id, "task_count": len(ranges), @@ -353,14 +356,14 @@ def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: return for start, end in ranges: - rebuild_project_derived_data_batch.delay( + generate_project_derived_data_batch.delay( project_id=project_id, group_id_start=start, group_id_end=end, ) logger.info( - "rebuild_project_derived_data.scheduled", + "generate_project_derived_data.scheduled", extra={ "project_id": project_id, "group_count": len(group_ids), @@ -370,18 +373,18 @@ def rebuild_project_derived_data(project_id: int, **kwargs: object) -> None: @instrumented_task( - name="sentry.issues.derived.tasks.rebuild_project_derived_data_batch", + name="sentry.issues.derived.tasks.generate_project_derived_data_batch", namespace=issues_tasks, silo_mode=SiloMode.CELL, processing_deadline_duration=int(BATCH_PROCESSING_DEADLINE.total_seconds()), ) -def rebuild_project_derived_data_batch( +def generate_project_derived_data_batch( project_id: int, group_id_start: int, group_id_end: int, **kwargs: object, ) -> None: - """Rebuild derived data for groups in [group_id_start, group_id_end). + """Generate derived data for groups in [group_id_start, group_id_end). Calls build_and_promote_derived_data for each group. On per-group GroupLogTimeout, re-enqueues the individual group for resumption and @@ -395,14 +398,14 @@ def rebuild_project_derived_data_batch( task_state = current_task() activation_id = task_state.id if task_state else None - if activation_id and already_spawned(_REBUILD_BATCH_TASK_KEY, activation_id): + if activation_id and already_spawned(_GENERATE_BATCH_TASK_KEY, activation_id): logger.info( - "rebuild_project_derived_data_batch.duplicate_skipped", + "generate_project_derived_data_batch.duplicate_skipped", extra={"project_id": project_id, "activation_id": activation_id}, ) metrics.incr( "taskworker.selfchain.duplicate_skipped", - tags={"task": _REBUILD_BATCH_TASK_KEY}, + tags={"task": _GENERATE_BATCH_TASK_KEY}, ) return @@ -429,44 +432,44 @@ def rebuild_project_derived_data_batch( processed += 1 except Group.DoesNotExist: logger.info( - "rebuild_project_derived_data_batch.group_not_found", + "generate_project_derived_data_batch.group_not_found", extra={"group_id": group_id, "project_id": project_id}, ) except GroupLogTimeout as e: # Re-enqueue the single group so the partially-drained # state is resumed, then continue the batch. - rid = e.rebuild_id - rebuild_group_derived_data.delay( + gen_id = e.generation_id + generate_group_derived_data.delay( group_id, - resume_invalidated_at=rid.invalidated_at.isoformat() - if rid and rid.invalidated_at + resume_generated_at=gen_id.generated_at.isoformat() + if gen_id and gen_id.generated_at else None, - resume_pipeline_hash=rid.pipeline_hash if rid else None, + resume_pipeline_hash=gen_id.pipeline_hash if gen_id else None, ) if time.monotonic() - start >= timeout_seconds: rescheduled = True metrics.incr( - "issues.derived.rebuild_batch_rescheduled", + "issues.derived.generate_batch_rescheduled", sample_rate=1.0, tags={"reason": "batch_timeout"}, ) - rebuild_project_derived_data_batch.delay( + generate_project_derived_data_batch.delay( project_id=project_id, group_id_start=group_id + 1, group_id_end=group_id_end, ) if activation_id: - mark_spawned(_REBUILD_BATCH_TASK_KEY, activation_id) + mark_spawned(_GENERATE_BATCH_TASK_KEY, activation_id) break metrics.incr( - "issues.derived.rebuild_project_groups_processed", + "issues.derived.generate_project_groups_processed", amount=processed, sample_rate=1.0, ) logger.info( - "rebuild_project_derived_data_batch.complete", + "generate_project_derived_data_batch.complete", extra={ "project_id": project_id, "group_id_start": group_id_start, diff --git a/src/sentry/issues/models/groupderiveddata.py b/src/sentry/issues/models/groupderiveddata.py index ac0180396566..1c93873b5c62 100644 --- a/src/sentry/issues/models/groupderiveddata.py +++ b/src/sentry/issues/models/groupderiveddata.py @@ -3,6 +3,7 @@ from datetime import UTC, datetime from django.db import models +from django.utils import timezone from sentry.backup.scopes import RelocationScope from sentry.db.models import ( @@ -29,15 +30,14 @@ class GroupDerivedData(DefaultFieldsModel): ~~~~~~~~~~~~~ The pipeline is deterministic: replaying the same log produces the same state. However, the log is not strictly append-only — historical entries - may be inserted, which is a primary reason rebuilds are triggered. + may be inserted, which is a primary reason generations are triggered. - * **invalidated_at** — set to ``now()`` by ``invalidate_group_derived_data`` - when the log is mutated in a way that requires a full rebuild. ``NULL`` - means the row is up-to-date. A rebuild reads ``invalidated_at`` at - start and clears it atomically on promotion via a CAS: - ``UPDATE SET invalidated_at=NULL ... WHERE invalidated_at=``. - If a newer invalidation arrived while the rebuild was running, the CAS - fails and the row stays flagged for another rebuild. + * **generated_at** — timestamp of when the generation that produced + this row's current state *started* processing. This is a CAS version: + incremental writes only succeed if ``generated_at`` hasn't changed + since the row was read, and generation promotes only succeed if their + ``generated_at`` is newer than the row's. The start time (rather + than finish time) reflects the log state the generation observed. * **cursor guard** — incremental writes only succeed if the writer's ``(cursor_date, cursor_id)`` is at or ahead of the row's, preventing @@ -47,14 +47,6 @@ class GroupDerivedData(DefaultFieldsModel): succeed if the pipeline version hasn't changed since the row was read. A pipeline upgrade invalidates in-flight incremental work. - Incremental processing writes per-batch with cursor and pipeline_hash - guards scoped to the row's ``id``. Rebuilds accumulate state in memory - and write once via ``promote_to_live``, which uses the invalidated_at - CAS and cursor guards. - - Groups needing a rebuild can be efficiently queried via - ``WHERE invalidated_at IS NOT NULL``. - See ``processing.py`` for the full lifecycle. """ @@ -62,11 +54,12 @@ class GroupDerivedData(DefaultFieldsModel): group = FlexibleForeignKey("sentry.Group", unique=True) - # NULL means the row is up-to-date. Non-NULL means a rebuild is needed; - # the timestamp identifies *which* invalidation request the rebuild must - # satisfy. Set by invalidate_group_derived_data, cleared atomically by - # promote_to_live. - invalidated_at = models.DateTimeField(null=True, default=None) + # Timestamp of when the generation that produced this state *started* + # processing. The start time (not finish time) reflects the log state + # the generation observed. Used as a CAS version — incremental writes + # check equality, generation promotes require their timestamp to be + # newer. Defaults to row creation time. + generated_at = models.DateTimeField(default=timezone.now, db_default=models.functions.Now()) cursor_date = models.DateTimeField(default=EPOCH) cursor_id = BoundedBigIntegerField(default=0) @@ -82,7 +75,7 @@ class GroupDerivedData(DefaultFieldsModel): # Stores the current Progress value as a string. progress = models.CharField(max_length=32, null=True, default="identified") - # The last time the above column was changed. + # The last time ``progress`` was changed. last_progressed_at = models.DateTimeField(null=True, default=None) # Pipeline hash stamped at row creation. If it doesn't match the current @@ -96,11 +89,6 @@ class Meta: indexes = [ models.Index(fields=["progress", "group"]), models.Index(fields=["last_progressed_at", "group"]), - models.Index( - fields=["invalidated_at"], - condition=models.Q(invalidated_at__isnull=False), - name="sentry_gdd_needs_rebuild", - ), ] - __repr__ = sane_repr("group_id", "invalidated_at", "cursor_date", "cursor_id") + __repr__ = sane_repr("group_id", "generated_at", "cursor_date", "cursor_id") diff --git a/src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py b/src/sentry/migrations/1141_add_generated_at_to_group_derived_data.py similarity index 53% rename from src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py rename to src/sentry/migrations/1141_add_generated_at_to_group_derived_data.py index 864c35db46bf..c0a2e19d49a6 100644 --- a/src/sentry/migrations/1141_add_invalidated_at_to_group_derived_data.py +++ b/src/sentry/migrations/1141_add_generated_at_to_group_derived_data.py @@ -1,5 +1,7 @@ # Generated by Django 5.2.14 +import django.db.models.functions.datetime +import django.utils.timezone from django.db import migrations, models from sentry.new_migrations.migrations import CheckedMigration @@ -15,15 +17,10 @@ class Migration(CheckedMigration): operations = [ migrations.AddField( model_name="groupderiveddata", - name="invalidated_at", - field=models.DateTimeField(null=True, default=None), - ), - migrations.AddIndex( - model_name="groupderiveddata", - index=models.Index( - condition=models.Q(("invalidated_at__isnull", False)), - fields=["invalidated_at"], - name="sentry_gdd_needs_rebuild", + name="generated_at", + field=models.DateTimeField( + default=django.utils.timezone.now, + db_default=django.db.models.functions.datetime.Now(), ), ), ] diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index c6e2f1ee0e80..308f49b7bd78 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -3942,16 +3942,6 @@ flags=FLAG_AUTOMATOR_MODIFIABLE, ) -# When True, derived data processing creates a live GroupDerivedData row -# on demand for groups that don't have one yet. When False, processing is -# a no-op until a backfill task explicitly creates and promotes a row. -register( - "issues.derived-data.create-on-demand", - type=Bool, - default=True, - flags=FLAG_AUTOMATOR_MODIFIABLE, -) - # Issues derived data — process_project_derived_data task # Number of groups per batch task when fanning out project-wide processing. register( diff --git a/tests/sentry/issues/derived/test_processing.py b/tests/sentry/issues/derived/test_processing.py index ea3b7a010be7..f79bafe2e735 100644 --- a/tests/sentry/issues/derived/test_processing.py +++ b/tests/sentry/issues/derived/test_processing.py @@ -103,7 +103,7 @@ def test_records_and_processes(self) -> None: assert entries[0].actor_id == user.id derived = process_group_log(group.id) - assert derived is not None + assert derived.cursor_id == entries[-1].id assert isinstance(derived.data, dict) @@ -113,12 +113,12 @@ def test_incremental_processing(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + first_cursor = derived.cursor_id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.cursor_id > first_cursor def test_noop_when_no_new_entries(self) -> None: @@ -127,11 +127,11 @@ def test_noop_when_no_new_entries(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + old_updated = derived.date_updated derived = process_group_log(group.id) - assert derived is not None + assert derived.date_updated == old_updated def test_process_group_log_only_affects_target(self) -> None: @@ -166,7 +166,6 @@ def test_batched_processing(self) -> None: # Process in batches of 2 — should take 3 batches (2+2+1) derived = process_group_log(group.id, batch_size=2) - assert derived is not None entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) assert derived.cursor_id == entries[-1].id @@ -232,7 +231,7 @@ def test_status_starts_open(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[STATUS] == IssueStatus.OPEN @@ -242,7 +241,7 @@ def test_resolve_closes(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.data["status"] == "closed" def test_unresolve_reopens(self) -> None: @@ -252,7 +251,7 @@ def test_unresolve_reopens(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.data["status"] == "open" def test_duplicate_resolve_ignored(self) -> None: @@ -262,7 +261,7 @@ def test_duplicate_resolve_ignored(self) -> None: _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.data["status"] == "closed" def test_duplicate_unresolve_ignored(self) -> None: @@ -271,7 +270,7 @@ def test_duplicate_unresolve_ignored(self) -> None: _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[STATUS] == IssueStatus.OPEN @@ -283,7 +282,7 @@ def test_status_toggle(self) -> None: _publish(group=group, action=UnresolveAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.data["status"] == "closed" # --- invalidation --- @@ -301,7 +300,6 @@ def test_hard_invalidate_with_cursor_deletes_if_past(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None invalidate_group_derived_data( group.id, cursor=(derived.cursor_date, derived.cursor_id), hard_delete=True @@ -312,7 +310,7 @@ def test_invalidate_with_cursor_noop_if_not_reached(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + old_cursor = derived.cursor_id future = derived.cursor_date.replace(year=derived.cursor_date.year + 1) @@ -326,29 +324,28 @@ def test_invalidate_then_reprocess(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.view_count == 2 invalidate_group_derived_data(group.id, hard_delete=True) derived = process_group_log(group.id) - assert derived is not None + assert derived.view_count == 2 # rebuilt from scratch - def test_invalidate_soft_flags_row(self) -> None: + def test_invalidate_soft_enqueues_generation(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + original_id = derived.id - with patch("sentry.issues.derived.processing.rebuild_group_derived_data") as mock_task: - invalidate_group_derived_data(group.id, hard_delete=False) + with patch("sentry.issues.derived.processing.generate_group_derived_data") as mock_task: + invalidate_group_derived_data(group.id) mock_task.delay.assert_called_once_with(group.id) - # Row is still in place but flagged for rebuild. + # Row is still in place — soft invalidation doesn't modify it. derived.refresh_from_db() assert derived.id == original_id - assert derived.invalidated_at is not None def test_resolved_in_pull_request_proposes_fix(self) -> None: group = self.create_group() @@ -360,7 +357,7 @@ def test_resolved_in_pull_request_proposes_fix(self) -> None: actor=GroupActionActor.user(user.id), ) derived = process_group_log(group.id) - assert derived is not None + # An open PR referencing the issue proposes a fix; the issue stays open. assert derived.data["status"] == "open" assert derived.progress == IssueProgressState.FIX_PROPOSED.value @@ -376,7 +373,7 @@ def test_pull_request_close_demotes_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) - assert derived is not None + assert derived.progress == IssueProgressState.FIX_PROPOSED.value _publish( @@ -385,7 +382,7 @@ def test_pull_request_close_demotes_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) - assert derived is not None + assert derived.progress == IssueProgressState.DIAGNOSED.value def test_pull_request_close_with_remaining_keeps_progress(self) -> None: @@ -403,7 +400,7 @@ def test_pull_request_close_with_remaining_keeps_progress(self) -> None: actor=actor, ) derived = process_group_log(group.id) - assert derived is not None + assert derived.progress == IssueProgressState.FIX_PROPOSED.value def test_pull_request_close_invalidate_and_replay_matches(self) -> None: @@ -422,7 +419,7 @@ def test_pull_request_close_invalidate_and_replay_matches(self) -> None: actor=actor, ) first = process_group_log(group.id) - assert first is not None + first_data = first.data.copy() first_progress = first.progress first_last_progressed_at = first.last_progressed_at @@ -440,7 +437,7 @@ def test_pipeline_hash_set_on_create(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + assert derived.pipeline_hash == PIPELINE.pipeline_hash def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: @@ -448,7 +445,7 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + first_cursor = derived.cursor_id # Insert a log entry directly to avoid inline processing from _publish @@ -473,14 +470,14 @@ def test_pipeline_hash_concurrent_change_skips_cursor_update(self) -> None: assert derived.cursor_id == first_cursor assert derived.pipeline_hash == "reset" - def test_invalidated_at_change_skips_incremental_write(self) -> None: + def test_generated_at_change_skips_incremental_write(self) -> None: from django.utils import timezone group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) derived = process_group_log(group.id) - assert derived is not None + first_cursor = derived.cursor_id GroupActionLogEntry.objects.create( @@ -493,15 +490,14 @@ def test_invalidated_at_change_skips_incremental_write(self) -> None: data={}, ) - # Simulate an invalidation between our read and the UPDATE in - # _process_batch — the row now has invalidated_at set. - GroupDerivedData.objects.filter(id=derived.id).update(invalidated_at=timezone.now()) + # Simulate a generation promoting between our read and the UPDATE + # in _process_batch — generated_at changed. + GroupDerivedData.objects.filter(id=derived.id).update(generated_at=timezone.now()) processing._process_batch(processing.PIPELINE, derived, 1) derived.refresh_from_db() assert derived.cursor_id == first_cursor - assert derived.invalidated_at is not None def test_invalidate_and_reprocess_restores_pipeline_hash(self) -> None: group = self.create_group() @@ -510,119 +506,113 @@ def test_invalidate_and_reprocess_restores_pipeline_hash(self) -> None: invalidate_group_derived_data(group.id, hard_delete=True) derived = process_group_log(group.id) - assert derived is not None - assert derived.pipeline_hash == PIPELINE.pipeline_hash - - def test_noop_when_on_demand_disabled(self) -> None: - group = self.create_group() - with self.options({"issues.derived-data.create-on-demand": False}): - # Publish with async strategy so inline processing doesn't create a row - with patch("sentry.issues.derived.processing.process_group_log_task"): - _publish( - group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id) - ) - - derived = process_group_log(group.id) - - assert derived is None - assert not GroupDerivedData.objects.filter(group_id=group.id).exists() + assert derived.pipeline_hash == PIPELINE.pipeline_hash -# --- Rebuild lifecycle --- +# --- Generation lifecycle --- @with_feature("projects:issue-action-log-write-to-db") class PromoteToLiveTest(TestCase): - def _soft_invalidate(self, group: Group) -> datetime: - """Soft-invalidate the GDD row and return the invalidated_at value.""" - invalidate_group_derived_data(group.id, hard_delete=False) - row = GroupDerivedData.objects.get(group_id=group.id) - assert row.invalidated_at is not None - return row.invalidated_at - - def test_hard_delete_rebuild_inserts_new_row(self) -> None: + def test_promote_inserts_when_no_row(self) -> None: + from django.utils import timezone + group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) + GroupDerivedData.objects.filter(group_id=group.id).delete() - # Hard-delete invalidation removes the row. - invalidate_group_derived_data(group.id, hard_delete=True) - assert not GroupDerivedData.objects.filter(group_id=group.id).exists() + gen_time = timezone.now() + candidate = GroupDerivedData( + group_id=group.id, generated_at=gen_time, cursor_date=EPOCH, cursor_id=0, data={} + ) + processing._drain_log(candidate, persist=False) + assert promote_to_live(candidate) is PromotionResult.PROMOTED - # Rebuild creates a new row via INSERT. - build_and_promote_derived_data(group.id) live = GroupDerivedData.objects.get(group_id=group.id) assert live.view_count == 1 - assert live.invalidated_at is None + assert live.generated_at == gen_time def test_promote_updates_existing_row(self) -> None: + from django.utils import timezone + group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) old = process_group_log(group.id) - assert old is not None + old_id = old.id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - inv_at = self._soft_invalidate(group) - candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) + gen_time = timezone.now() + candidate = GroupDerivedData( + group_id=group.id, generated_at=gen_time, cursor_date=EPOCH, cursor_id=0, data={} + ) processing._drain_log(candidate, persist=False) - assert promote_to_live(candidate, invalidated_at=inv_at) is PromotionResult.PROMOTED + assert promote_to_live(candidate) is PromotionResult.PROMOTED live = GroupDerivedData.objects.get(group_id=group.id) assert live.id == old_id assert live.view_count == 2 - assert live.invalidated_at is None # cleared by promotion + assert live.generated_at == gen_time def test_promote_rejected_if_cursor_behind(self) -> None: + from django.utils import timezone + group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - live = process_group_log(group.id) - assert live is not None - inv_at = self._soft_invalidate(group) + # Stamp a generated_at on the live row so the candidate can't + # win just by having a non-NULL generated_at. + gen_time = timezone.now() + process_group_log(group.id) + live = GroupDerivedData.objects.get(group_id=group.id) + GroupDerivedData.objects.filter(id=live.id).update(generated_at=gen_time) + live.refresh_from_db() - # Candidate only processes the first entry. - candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) + # Candidate uses the same generated_at but only processes one entry. + candidate = GroupDerivedData( + group_id=group.id, generated_at=gen_time, cursor_date=EPOCH, cursor_id=0, data={} + ) processing._process_batch(PIPELINE, candidate, batch_size=1, persist=False) assert (candidate.cursor_date, candidate.cursor_id) < ( live.cursor_date, live.cursor_id, ) - assert promote_to_live(candidate, invalidated_at=inv_at) is PromotionResult.CURSOR_BEHIND + assert promote_to_live(candidate) is PromotionResult.CURSOR_BEHIND - def test_promote_superseded_by_newer_invalidation(self) -> None: + def test_promote_superseded_by_newer_generation(self) -> None: from django.utils import timezone group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) process_group_log(group.id) - old_inv_at = self._soft_invalidate(group) - - # A second invalidation arrives — the row now has a newer timestamp. - GroupDerivedData.objects.filter(group_id=group.id).update(invalidated_at=timezone.now()) + # Simulate a newer generation having already promoted. + newer_time = timezone.now() + GroupDerivedData.objects.filter(group_id=group.id).update(generated_at=newer_time) - candidate = GroupDerivedData(group_id=group.id, cursor_date=EPOCH, cursor_id=0, data={}) + # An older generation tries to promote. + old_time = newer_time - timedelta(seconds=10) + candidate = GroupDerivedData( + group_id=group.id, generated_at=old_time, cursor_date=EPOCH, cursor_id=0, data={} + ) processing._drain_log(candidate, persist=False) - assert promote_to_live(candidate, invalidated_at=old_inv_at) is PromotionResult.SUPERSEDED + assert promote_to_live(candidate) is PromotionResult.SUPERSEDED def test_build_and_promote(self) -> None: group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(self.user.id)) - process_group_log(group.id) - invalidate_group_derived_data(group.id, hard_delete=False) build_and_promote_derived_data(group.id) - derived = GroupDerivedData.objects.get(group_id=group.id) assert derived.view_count == 1 assert derived.data["status"] == "closed" - assert derived.invalidated_at is None + assert derived.generated_at is not None def test_build_and_promote_updates_existing_row(self) -> None: group = self.create_group() @@ -630,26 +620,62 @@ def test_build_and_promote_updates_existing_row(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) old = process_group_log(group.id) - assert old is not None + old_id = old.id _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) - invalidate_group_derived_data(group.id, hard_delete=False) build_and_promote_derived_data(group.id) live = GroupDerivedData.objects.get(group_id=group.id) assert live.id == old_id assert live.view_count == 2 + assert live.generated_at is not None - def test_build_and_promote_noop_when_not_invalidated(self) -> None: + def test_generation_prevents_stale_incremental_write(self) -> None: + """End-to-end ABA test: incremental write computed from pre-generation + state must not overwrite a generation's result.""" group = self.create_group() _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - process_group_log(group.id) - # No invalidation — build_and_promote should be a no-op. + # Incremental processing reads the row. + derived = process_group_log(group.id) + + pre_gen_generated_at = derived.generated_at # None (never generated) + + # A generation runs and promotes (stamps generated_at). build_and_promote_derived_data(group.id) - derived = GroupDerivedData.objects.get(group_id=group.id) - assert derived.invalidated_at is None + derived.refresh_from_db() + assert derived.generated_at is not None + + # A new entry arrives. + GroupActionLogEntry.objects.create( + group_id=group.id, + project_id=group.project_id, + type=GroupActionType.VIEW, + actor_type=GroupActorType.SYSTEM, + actor_id=0, + source=SOURCE, + data={}, + ) + + # Create a stale incremental writer with the pre-generation state. + stale = GroupDerivedData( + group_id=group.id, + generated_at=pre_gen_generated_at, + cursor_date=derived.cursor_date, + cursor_id=derived.cursor_id, + data=derived.data.copy(), + pipeline_hash=derived.pipeline_hash, + ) + # The stale writer processes the new entry. + processing._process_batch(PIPELINE, stale, batch_size=1) + + # The write should have been rejected because generated_at changed. + derived.refresh_from_db() + assert derived.generated_at is not None + # Cursor should NOT have advanced (stale write rejected). + entries = list(GroupActionLogEntry.objects.filter(group_id=group.id).order_by("id")) + assert derived.cursor_id == entries[-2].id # still at the pre-new-entry position def test_drain_log_respects_time_limit(self) -> None: group = self.create_group() @@ -674,58 +700,52 @@ def test_process_group_log_reenqueues_on_partial_drain(self) -> None: patch("sentry.issues.derived.processing._drain_log", return_value=False), patch("sentry.issues.derived.processing.process_group_log_task") as mock_task, ): - derived = process_group_log(group.id) + process_group_log(group.id) - assert derived is not None mock_task.delay.assert_called_once_with(group.id) def test_build_and_promote_caches_on_timeout_for_resumption(self) -> None: group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - process_group_log(group.id) - invalidate_group_derived_data(group.id, hard_delete=False) with patch("sentry.issues.derived.processing._drain_log", return_value=False): with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data(group.id) assert exc_info.value.group_id == group.id - assert exc_info.value.rebuild_id is not None + assert exc_info.value.generation_id is not None # Resuming completes the promotion. - build_and_promote_derived_data(group.id, rebuild_id=exc_info.value.rebuild_id) + build_and_promote_derived_data(group.id, generation_id=exc_info.value.generation_id) promoted = GroupDerivedData.objects.get(group_id=group.id) assert promoted.view_count == 5 - assert promoted.invalidated_at is None - def test_resumed_rebuild_advances_cursor_on_repeat_timeout(self) -> None: - from sentry.issues.derived.processing import _rebuild_cache + def test_resumed_generation_advances_cursor_on_repeat_timeout(self) -> None: + from sentry.issues.derived.processing import _generation_cache group = self.create_group() for _ in range(5): _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(self.user.id)) - process_group_log(group.id) - invalidate_group_derived_data(group.id, hard_delete=False) with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data(group.id, batch_size=2, time_limit=timedelta(0)) - rid = exc_info.value.rebuild_id - assert rid is not None - state = _rebuild_cache.get(rid) + gen_id = exc_info.value.generation_id + assert gen_id is not None + state = _generation_cache.get(gen_id) assert state is not None first_cursor = state.cursor_id assert first_cursor > 0 with pytest.raises(GroupLogTimeout) as exc_info: build_and_promote_derived_data( - group.id, rebuild_id=rid, batch_size=2, time_limit=timedelta(0) + group.id, generation_id=gen_id, batch_size=2, time_limit=timedelta(0) ) - rid2 = exc_info.value.rebuild_id - assert rid2 is not None - state = _rebuild_cache.get(rid2) + gen_id2 = exc_info.value.generation_id + assert gen_id2 is not None + state = _generation_cache.get(gen_id2) assert state is not None assert state.cursor_id > first_cursor @@ -933,7 +953,6 @@ def test_round_trip_preserves_state(self) -> None: _publish(group=group, action=ViewAction(), actor=GroupActionActor.user(user.id)) _publish(group=group, action=ResolveAction(), actor=GroupActionActor.user(user.id)) first = process_group_log(group.id) - assert first is not None first_data = first.data.copy() first_view_count = first.view_count @@ -986,7 +1005,6 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=ViewAction(), actor=actor) derived = process_group_log(group.id) - assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] == IssueProgressState.IDENTIFIED @@ -994,7 +1012,6 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=ResolveAction(), actor=actor) derived = process_group_log(group.id) - assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] is None @@ -1002,7 +1019,6 @@ def test_progress_round_trip(self) -> None: _publish(group=group, action=UnresolveAction(), actor=actor) derived = process_group_log(group.id) - assert derived is not None state = GroupDerivedDataStore.load(PIPELINE, derived) assert state[PROGRESS] == IssueProgressState.IDENTIFIED @@ -1069,5 +1085,5 @@ def test_completes_with_generous_timeout(self) -> None: GroupDerivedData.objects.filter(group_id=group.id).delete() derived = process_group_log(group.id, timeout=timedelta(minutes=5)) - assert derived is not None + assert derived.view_count == 3 diff --git a/tests/sentry/tasks/test_merge.py b/tests/sentry/tasks/test_merge.py index fa5032f5a43e..70dc2555136a 100644 --- a/tests/sentry/tasks/test_merge.py +++ b/tests/sentry/tasks/test_merge.py @@ -286,10 +286,9 @@ def test_merge_invalidates_derived_data(self) -> None: assert Group.objects.filter(id=old_group.id).exists() is False - # The derived data is soft-invalidated: the row is flagged for - # rebuild but continues serving reads until the rebuild completes. + # The generation task ran and stamped a new generated_at. derived.refresh_from_db() - assert derived.invalidated_at is not None + assert derived.generated_at is not None @mock_redis_buffer() def test_merge_original_group_id(self) -> None: