Switches from custom process pool to ProcessPoolExecutor. by ellisonmarks · Pull Request #124235 · getsentry/sentry

ellisonmarks · 2026-09-12T00:50:42Z

Aside from using a stdlib component, this gives us access to the max_tasks_per_child parameter, so we can restart the workers after they process a defined number of tasks. The idea here is to control memory growth in the worker processes after handling many tasks. Starting at 100 as a sort of arbitrary number.

sentry · 2026-09-12T00:53:58Z


    # Ensure all tasks are completed before exiting
-    task_queue.join()
+    future_wait(futures)


Bug: The code uses future_wait but doesn't check for exceptions in completed futures. If a worker process crashes, associated cleanup tasks will silently fail, leading to incomplete data deletion.
_{Severity: HIGH}

Suggested Fix

After future_wait(futures) returns, iterate through the futures list and call .result() or .exception() on each one to check for and handle any exceptions, particularly BrokenProcessPool. This will ensure that failures in worker processes are detected and logged, preventing silent data loss. Alternatively, consider replacing the executor.submit loop and wait call with executor.map, which propagates exceptions from workers.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: src/sentry/runner/commands/cleanup.py#L892 Potential issue: The cleanup process uses a `ProcessPoolExecutor` to run deletion tasks in parallel and waits for them to complete using `future_wait` (an alias for `concurrent.futures.wait`). If a worker process crashes due to an OOM error or segfault, any futures assigned to it will fail with a `BrokenProcessPool` exception. However, the `future_wait` function does not propagate exceptions from the futures. Since the code never checks the result of individual futures via `.result()` or `.exception()`, these failures are silently ignored. This results in cleanup tasks being skipped, leading to incomplete data deletion and violation of data retention policies.

Also affects:

src/sentry/runner/commands/cleanup.py:953~953

src/sentry/runner/commands/cleanup.py:1019~1019

_{Did we get this right? 👍 / 👎 to inform future reviews.}

Neither did the original code, and I don't intend to change that in this PR.

github-actions · 2026-09-12T00:55:10Z

Backend Test Failures

Failures on 716f2ff in this run:

tests/sentry/runner/commands/test_cleanup.py::RunBulkQueryDeletesByProjectTest::test_project_id_passed_to_task_queue — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:230: in test_project_id_passed_to_task_queue
    run_bulk_deletes_by_project(
E   TypeError: run_bulk_deletes_by_project() got an unexpected keyword argument 'task_queue'

tests/sentry/runner/commands/test_cleanup.py::RunBulkQueryDeletesByProjectTest::test_run_bulk_query_deletes_by_project — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:184: in test_run_bulk_query_deletes_by_project
    run_bulk_deletes_by_project(
E   TypeError: run_bulk_deletes_by_project() got an unexpected keyword argument 'task_queue'

tests/sentry/runner/commands/test_cleanup.py::SeerRunCleanupTest::test_cascades_to_children — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:367: in test_cascades_to_children
    assert not SeerRun.objects.filter(id=run.id).exists()
E   AssertionError: assert not True
E    +  where True = exists()
E    +    where exists = <BaseQuerySet [<SeerRun at 0x7f54a378ce60: id=6, organization_id=4558818694266912, seer_run_state_id=None, type='explorer', referrer=None>]>.exists
E    +      where <BaseQuerySet [<SeerRun at 0x7f54a378ce60: id=6, organization_id=4558818694266912, seer_run_state_id=None, type='explorer', referrer=None>]> = filter(id=6)
E    +        where filter = <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400>.filter
E    +          where <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400> = SeerRun.objects
E    +        and   6 = <SeerRun at 0x7f54a378df40: id=6, organization_id=4558818694266912, seer_run_state_id=None, type=SeerRunType.EXPLORER, referrer=None>.id

tests/sentry/runner/commands/test_cleanup.py::SeerRunCleanupTest::test_deletes_runs_older_than_retention — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:352: in test_deletes_runs_older_than_retention
    assert not SeerRun.objects.filter(id=old.id).exists()
E   AssertionError: assert not True
E    +  where True = exists()
E    +    where exists = <BaseQuerySet [<SeerRun at 0x7f54a3308950: id=17, organization_id=4558818694332448, seer_run_state_id=None, type='explorer', referrer=None>]>.exists
E    +      where <BaseQuerySet [<SeerRun at 0x7f54a3308950: id=17, organization_id=4558818694332448, seer_run_state_id=None, type='explorer', referrer=None>]> = filter(id=17)
E    +        where filter = <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400>.filter
E    +          where <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400> = SeerRun.objects
E    +        and   17 = <SeerRun at 0x7f54a38d4a10: id=17, organization_id=4558818694332448, seer_run_state_id=None, type=SeerRunType.EXPLORER, referrer=None>.id

tests/sentry/runner/commands/test_cleanup.py::SeerRunCleanupTest::test_night_shift_links_survive_with_null_seer_run — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:386: in test_night_shift_links_survive_with_null_seer_run
    assert not SeerRun.objects.filter(id=run.id).exists()
E   AssertionError: assert not True
E    +  where True = exists()
E    +    where exists = <BaseQuerySet [<SeerRun at 0x7f54a330b110: id=24, organization_id=4558818694463520, seer_run_state_id=None, type='explorer', referrer=None>]>.exists
E    +      where <BaseQuerySet [<SeerRun at 0x7f54a330b110: id=24, organization_id=4558818694463520, seer_run_state_id=None, type='explorer', referrer=None>]> = filter(id=24)
E    +        where filter = <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400>.filter
E    +          where <sentry.db.models.manager.base.BaseManager object at 0x7f54b8e15400> = SeerRun.objects
E    +        and   24 = <SeerRun at 0x7f54a33081d0: id=24, organization_id=4558818694463520, seer_run_state_id=None, type=SeerRunType.EXPLORER, referrer=None>.id

tests/sentry/runner/commands/test_cleanup.py::ExpiryDeletionsCodePathTest::test_expiry_cleanup_ignores_days_flag — log

[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/runner/commands/test_cleanup.py:503: in test_expiry_cleanup_ignores_days_flag
    assert not EventAttachment.objects.filter(id=expired.id).exists()
E   AssertionError: assert not True
E    +  where True = exists()
E    +    where exists = <BaseQuerySet [<EventAttachment at 0x7f54a31ee210: id=11, event_id='78de7a849add4adaac7e2bc76ae25e64', name='expired.txt'>]>.exists
E    +      where <BaseQuerySet [<EventAttachment at 0x7f54a31ee210: id=11, event_id='78de7a849add4adaac7e2bc76ae25e64', name='expired.txt'>]> = filter(id=11)
E    +        where filter = <sentry.db.models.manager.base.BaseManager object at 0x7f54ba0bae40>.filter
E    +          where <sentry.db.models.manager.base.BaseManager object at 0x7f54ba0bae40> = <class 'sentry.models.eventattachment.EventAttachment'>.objects
E    +        and   11 = <EventAttachment at 0x7f54a31ed0d0: id=11, event_id='78de7a849add4adaac7e2bc76ae25e64', name='expired.txt'>.id

cursor

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit bf9014a. Configure here.}

cursor · 2026-09-12T00:57:21Z

    for chunk in q.iterator(chunk_size=DELETES_BY_PROJECT_CHUNK_SIZE):
-        task_queue.put((imp, chunk, project_id))
-        chunk_count += 1
+        futures.append(executor.submit(task_execution, imp, chunk, project_id))


Unbounded futures can exhaust memory

High Severity

Every chunk is now submitted up front and retained as a Future, with no replacement for the old Queue(1000) backpressure. run_bulk_deletes_by_project and run_bulk_deletes_in_deletes wait only after scheduling all models and projects, so a large cleanup can hold millions of chunk tuples in the parent and OOM it.

Additional Locations (2)

src/sentry/runner/commands/cleanup.py#L914-L953

src/sentry/runner/commands/cleanup.py#L864-L892

^{Reviewed by Cursor Bugbot for commit bf9014a. Configure here.}

cursor · 2026-09-12T00:57:22Z


    # Ensure all tasks are completed before exiting
-    task_queue.join()
+    future_wait(futures)


Worker failures are silently ignored

Medium Severity

future_wait only waits for completion and never inspects Future results. Process crashes, pickling errors, and initializer failures (BrokenProcessPool) therefore finish as success, so chunks can be skipped while cleanup still reports that it completed.

Additional Locations (2)

src/sentry/runner/commands/cleanup.py#L951-L953

src/sentry/runner/commands/cleanup.py#L1017-L1019

^{Reviewed by Cursor Bugbot for commit bf9014a. Configure here.}

github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 12, 2026

sentry Bot reviewed Sep 12, 2026

View reviewed changes

cursor Bot reviewed Sep 12, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

Switches from custom process pool to ProcessPoolExecutor. - #124235

Switches from custom process pool to ProcessPoolExecutor.#124235
ellisonmarks wants to merge 1 commit into
masterfrom
emarks/cleanup-concurrent-futures

ellisonmarks commented Sep 12, 2026

Uh oh!

sentry Bot Sep 12, 2026

Uh oh!

ellisonmarks Sep 12, 2026

Uh oh!

github-actions Bot commented Sep 12, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Sep 12, 2026

Uh oh!

cursor Bot Sep 12, 2026

Uh oh!

ellisonmarks Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

ellisonmarks commented Sep 12, 2026

Uh oh!

sentry Bot Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ellisonmarks Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 12, 2026

Backend Test Failures

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 12, 2026

Choose a reason for hiding this comment

Unbounded futures can exhaust memory

Uh oh!

cursor Bot Sep 12, 2026

Choose a reason for hiding this comment

Worker failures are silently ignored

Uh oh!

ellisonmarks Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant