fix(integrations): Chunk the repo-activity RPC in sync_repos_for_org - #120728
Draft
billyvg wants to merge 1 commit into
Draft
fix(integrations): Chunk the repo-activity RPC in sync_repos_for_org#120728billyvg wants to merge 1 commit into
billyvg wants to merge 1 commit into
Conversation
`_sync_repos_for_org` passed the entire removed-repo set to `repository_service.find_recently_active_repo_external_ids` in a single cross-silo RPC. For orgs with ~10K repos on the provider side that is a multi-tens-of-KB request body built from an unbounded list, and the call fails (timeout / request-size limit / memory spike on the cell side) before any repo is disabled, so the sync never converges. Chunk the candidate list and accumulate the results instead. The rest of the task already batches its work through `chunked(...)`; this was the last unbounded payload on the path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017YpUohVVykHZnEEdAS77dw
Contributor
Backend Test FailuresFailures on
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes an unbounded cross-silo RPC payload in the periodic SCM repo sync.
1986a344223545f0af045736530b1efbsentry.integrations.source_code_management.sync_repos.sync_repos_for_org(envcontrol)Root cause
_sync_repos_for_orgdiffs the provider's repo list against Sentry'sRepositoryrows, then — before disabling anything — asks the cell silo which of the removed repos still have recent commit/PR/code-review activity:removed_idsis unbounded — it issentry_active_ids - provider_external_ids. For a GitHub org with ~10K repos (99+ pages of/installation/repositories), that single call serializes every removed external id into one RPC POST body (~72KB on the failing event). Nothing caps it, so at scale the call dies on request-size limit / timeout / cell-side memory before a single repo is disabled, and the task never converges — every scheduled run re-does the same doomed request. The task'sprocessing_deadline_duration=120plusRetry(times=3)means it just burns retries.Everything else in this task already batches through
chunked(...)(create_repos_batch,disable_repos_batch,restore_repos_batchall dispatch inSYNC_BATCH_SIZEslices). This activity lookup was the one remaining unbounded payload on the path.Fix
Chunk the candidate list and accumulate the results:
Notes on the shape of the change:
ACTIVITY_LOOKUP_BATCH_SIZE = 500rather than reusingSYNC_BATCH_SIZE = 100.SYNC_BATCH_SIZEsizes async task dispatch, where extra batches are cheap. This lookup is a blocking round trip inside a task with a 120s processing deadline, so 100 would mean ~99 sequential RPCs for a 9.9K-repo org. 500 keeps each body around 4KB while holding the worst case to ~20 calls.sorted(...)instead oflist(...)makes batch boundaries deterministic (easier to correlate logs/retries) and narrows the element type tostr—RpcRepository.external_idisstr | None, and theif eid is not Nonefilter is a no-op at runtime sinceremoved_idsis built only from repos with a truthyexternal_id.if removed_id_list:guard is gone because the loop body simply doesn't execute for an empty set; thefind_recently_active_repo_external_idsimpl also already short-circuits on an empty list. The logging/metrics block moved out one level, which is behaviour-preserving (active_skippedis empty whenever there were no candidates).Ruled out
querybuilder.name: UnresolvedQueryon the event is a red herring — scope contamination from a previous task in the same worker process. No query-builder code is reachable from this task.installation.get_repositories(raise_on_page_limit=True)already has a pagination cap and itsApiPaginationTruncatedpath is handled (partial results are used for create/restore and the disable path is skipped). That's the path that would produce a provider failure, and the event isn't it._halt_broken_integrationand halt without an issue, so this error is a genuine failure, not a known terminal state.DISABLE_ACTIVITY_CUTOFF_DAYSexists to prevent. Chunking preserves the exact result set.Known remaining risk (not addressed here)
repository_service.get_repositories(organization_id=..., integration_id=..., providers=[...])has no limit or pagination (src/sentry/integrations/services/repository/impl.py) — it serializes every matchingRepositoryrow into the RPC response (~78KB for the org in this event). It's a read, so it doesn't have the same failure mode as the oversized request body, but it will keep growing with repo count, anddisable_repos_batch/restore_repos_batcheach call it again per batch. Fixing it properly means a paginated orexternal_ids-filtered variant of the RPC method, which is a bigger change than this hotfix and is deliberately out of scope.Tests
Added
test_activity_lookup_is_chunkedtotests/sentry/integrations/source_code_management/test_sync_repos.py. It patchesACTIVITY_LOOKUP_BATCH_SIZEdown to 2, removes 5 repos from the provider listing (one of which has a recent commit), spies on the RPC while delegating to the real implementation, and asserts:ACTIVE, the other four end upDISABLED..venv, nodevenv, no Django installed), sopytestandprekcould not be executed. What was verified locally:ruff checkandruff format --checkclean on both files,py_compileclean, and the chunk/accumulate logic exercised standalone against a 9,900-id set (20 calls, max 500 ids per call, correct skip/disable partition). CI needs to confirm the pytest run.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
🤖 Generated with Claude Code
https://claude.ai/code/session_017YpUohVVykHZnEEdAS77dw
Generated by Claude Code