fix(bitbucket-cloud): tolerate denied repos, cut request budget - #2011
Merged
Conversation
A repository can be returned by the workspace listing and still answer 403 to every request under it — routine with repo-scoped tokens and per-repository permissions, and confirmed in production on virtuozzo. Those fetches raise: refs/branches, the commit ranges, commit diffstat and the pull-request listing all go through paginate(), which only tolerates 404. The per-repository quarantine caught the error and let the other repositories finish, but finish_bucket then failed the stream at the end of the sync. A 403 is permanent, so that repeated on every run: the sync stayed red forever and buried the transient failures the quarantine exists to surface. Split the two error classes. 403 and 404 on a repository now mark it inaccessible on the shared catalog and skip it, logged once as a warning and summarised at the end of the sync; every other error keeps the previous behaviour and still fails the sync. Because the catalog is shared by all streams, the first stream to hit the 403 saves the rest from rediscovering it. 404 is included deliberately: a repository listed at the start of a sync can be deleted while it runs. branches needs one extra step. It is a bucket-scoped, deletion-aware snapshot, so a skipped repository contributing no branches would read as "every branch of that repository was deleted" and drop live rows. Its marker is therefore unavailable when any repository in the bucket was skipped or failed, which makes dbt keep the previous generation. The cost is that the other repositories in that bucket keep their previous branch generation until the denied one becomes readable; their commits and pull requests are unaffected, being separate streams. This does not change snapshot_available on the optional per-entity fetches (comments, diffstat, activity, tasks, issues, pipelines). Those already tolerate 403 and mark the generation unavailable, which is what stops a denied read from being recorded as an empty collection and wiping previously synced rows — that behaviour was correct and is untouched. tests: 11 new cases pin denied-is-skipped against transient-still-fails, the catalog hand-off between streams, and the branches marker in both directions. Verified they catch the regression: narrowing DENIED_STATUSES to the empty set fails 5 of them. 117 pass. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Contributor
|
📝 WalkthroughWalkthroughBitbucket Cloud syncs now classify repository access denials as skips, track inaccessible repositories, gate unchanged repositories, scope branch snapshots per repository, tolerate unavailable feature data, chunk commit comparisons, and share slim selections across streams. ChangesBitbucket Cloud reliability and request budgeting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Stream
participant BitbucketClient
participant RepositoryCatalog
participant SnapshotModel
Stream->>BitbucketClient: fetch repository data
BitbucketClient-->>Stream: denied or unavailable response
Stream->>RepositoryCatalog: mark inaccessible or record unavailable feature
Stream-->>SnapshotModel: emit scoped records and completeness markers
SnapshotModel-->>SnapshotModel: select latest repository generation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/base.py`:
- Around line 199-212: Update the inaccessible-repository branch in the base
stream’s record-reading loop to append each pre-known inaccessible repo to
self._skipped_repositories before continuing, matching
BranchesStream.read_records and ensuring finish_bucket includes it in the
skipped summary. Do not alter the existing BitbucketApiError classification or
other exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4df75515-33b1-4fb6-b3ab-fa29b0b869e4
📒 Files selected for processing (5)
src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/client.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/base.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/branches.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/conftest.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py
cyberantonz
approved these changes
Jul 29, 2026
…ve all streams
Audit of every HTTP call site found two fetches that still raised on 403 and,
via the new denied-repository handling, would have marked the whole repository
inaccessible over a feature-level denial — suppressing its commits and pull
requests because the inaccessible mark is shared across streams:
- the open-pipeline refetch (pipelines/{uuid})
- pipeline step test reports
Both are per-repository features; a 403 there means "no pipelines visible",
not "this repository is unreadable". They now tolerate 403/404 like every
other optional fetch, and the snapshot marker records the denial.
The guarantee is now enforced structurally rather than by enumeration: a test
matrix derives the stream list from SourceBitbucketCloud.streams() itself and
runs every stream — current and future — against a repository that answers 403
to everything (sync must stay green, no items emitted, every marker touching
that repository unavailable, state never advanced) and against a hard 500
(sync must still fail loudly). Two targeted tests pin the feature-level paths
the matrix cannot reach, verified to fail without the fix. 173 pass.
Two matrix findings were legitimate behaviour, asserted as such: the
repositories stream emits the denied repository's metadata (the workspace
listing was readable — only the contents are not), and empty buckets keep
available markers (their partitions contain nothing to delete).
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
enabled auto-merge
July 29, 2026 08:51
aleksdotbar
disabled auto-merge
July 29, 2026 08:54
…und by review
A systematic review of every call site against the operational realities the
pre-rewrite connector already knew about (its ignore_404 comment names them)
found four more members of the same class as the 403 bug — foreseeable
conditions handled as exceptional failures:
1. Vanished commit diffstat. diffstat/{sha} can be permanently gone (orphaned
merge parents, rewritten history); it raised, so one such commit failed its
repository on every sync forever. It now goes through the tolerant fetch and
the snapshot marker records the denial — the completeness gate keeps what
was known instead of zeroing the commit's line counts. The repository still
advances past the bad commit.
2. Bucket-scoped branch generations starve at fleet scale. One denied
repository froze branch updates for its whole bucket; with denied
repositories scattered across buckets — reported as common in production —
every bucket freezes and branches never update again. Generations are now
per repository (stream and dbt model): a denied repository simply produces
no marker and keeps its previous generation, every other repository updates
independently. Verified on ClickHouse 25.7.5: a newer generation replaces
(deletions intact), a denied repository retains, neighbours are unaffected.
Trade: branches of a repository deleted from the workspace linger; bounded,
and preferable to fleet-wide starvation.
3. Credential failure drowned in quarantine noise. A 401 is global, not
per-repository; it was quarantined 1,400 times and surfaced as a generic
end-of-sync error. It now aborts at the first occurrence with the actionable
cause (unset bitbucket_username sends Atlassian tokens as Bearer; or the
token expired/rotated).
4. Unbounded include/exclude form. Bitbucket documents no ceiling on commit
range parameters and its limits surface as unexplained 400s (BCLOUD-13229);
a repository with hundreds of branches sent them all in one form. Includes
are now chunked at 100 with the full exclude set on every chunk — the union
of the chunked ranges is the same commit set, and bronze dedups overlap.
Each fix carries a test that fails without it; the all-denied matrix now also
exercises the diffstat path. 177 pass.
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
changed the title
aleksdotbar
changed the title
A repository marked inaccessible by an earlier stream was skipped silently, so later streams' end-of-sync summaries under-reported what they skipped. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
enabled auto-merge
July 29, 2026 09:37
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py (1)
313-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion passes for state that did advance.
The disjunction only rules out
head_shas/updated_on; aPipelineStateStreamsubclass that wrote{"created_on": ...}for the denied repository would still satisfy it. WithFullyDeniedClientnothing reachescommit_repository_state, so the strict form is both simpler and green.♻️ Stronger, simpler assertion
- assert stream.state["repositories"].get(repo_state_key(repo), {}) in ({}, None) or ( - "head_shas" not in stream.state["repositories"].get(repo_state_key(repo), {}) - and "updated_on" not in stream.state["repositories"].get(repo_state_key(repo), {}) - ), f"{stream_class.__name__} advanced state for a repository it never read" + assert repo_state_key(repo) not in stream.state["repositories"], ( + f"{stream_class.__name__} advanced state for a repository it never read" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py` around lines 313 - 326, Strengthen the final assertion in test_denied_repository_state_never_advances to require that the denied repository’s state entry is exactly absent or empty ({}/None), rather than allowing entries containing other fields such as created_on. Keep the existing FullyDeniedClient setup and use the repository state lookup already present in the test.src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__repository_branches.sql (1)
11-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winScope branch generations by
repository_uuidinstead of(workspace, repo_slug).
repository_uuidis emitted by the item and completion records, while Bitbucket workspaces and repo slugs can change. Renaming a repo updates those fields but leaves the old generation scoped to the stale key, so both old and new rows can map to the same or orphaned generations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__repository_branches.sql` around lines 11 - 37, The generations and latest CTEs currently scope branches by workspace and repo_slug; update both grouping and selected repository identity fields to use repository_uuid from the records instead. Keep tenant_id, source_id, generation completion, and latest-generation selection behavior unchanged while ensuring renamed repositories remain associated with their existing generations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__repository_branches.sql`:
- Line 53: Update the join with latest in the repository branches query to avoid
nullable workspace and repo_slug keys. Match generations using the deterministic
non-NULL repository_uuid together with generation_id, or use ClickHouse’s
isNotDistinctFrom semantics for nullable keys, while preserving tenant and
source scoping.
In
`@src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/client.py`:
- Around line 267-273: Update the commit pagination logic in the relevant method
to return immediately whenever includes is empty, regardless of excludes. Remove
the excludes-only paginate call, while preserving the existing return when both
includes and excludes are empty and normal pagination when includes is present.
---
Nitpick comments:
In
`@src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__repository_branches.sql`:
- Around line 11-37: The generations and latest CTEs currently scope branches by
workspace and repo_slug; update both grouping and selected repository identity
fields to use repository_uuid from the records instead. Keep tenant_id,
source_id, generation completion, and latest-generation selection behavior
unchanged while ensuring renamed repositories remain associated with their
existing generations.
In
`@src/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py`:
- Around line 313-326: Strengthen the final assertion in
test_denied_repository_state_never_advances to require that the denied
repository’s state entry is exactly absent or empty ({}/None), rather than
allowing entries containing other fields such as created_on. Keep the existing
FullyDeniedClient setup and use the repository state lookup already present in
the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d11a7cf6-76a3-438d-9343-82b59aee4eee
📒 Files selected for processing (8)
src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__repository_branches.sqlsrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/client.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/base.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/branches.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/file_changes.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/metric_events.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_file_changes.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/base.py
mitasovr
approved these changes
Jul 29, 2026
aleksdotbar
disabled auto-merge
July 29, 2026 10:16
Bitbucket's ~1,000 req/h budget cannot cover ~1,400 repositories when every stream re-lists everything each sync: - idle gate: repositories whose updated_on (free with the workspace listing) is unchanged since the last pass cost zero requests in the push-driven streams (branches, commits, file_changes, reachability) - shared selections: PR / pipeline / issue listings run once per repository per sync; the six PR streams, three pipeline streams and three issue streams reuse a slim projection (id, cursor, head hashes; ~100 bytes per entity) cached on the shared catalog, keyed by watermark so a lagging stream still fetches its own An idle repository now costs ~5 requests per sync instead of ~13-25. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
changed the title
aleksdotbar
enabled auto-merge
July 29, 2026 10:49
POST /commits falls back to every branch when the form carries no include, so a repository whose branches have all disappeared paged its entire history back out under the exclude set. No current head means nothing is newly reachable. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdle-gate check duplicated across four streams. The same
repo_updated_on-vs-priorguard (byte-identical, including the comment, in three of the four files) is reimplemented separately instead of living once onBitbucketIncrementalStream. A future correction to this gating rule (e.g. a subtle edge case in howupdated_onis compared) would need to be applied in four places and could easily be missed in one.
src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py#L17-L24: extract this block into a shared base helper, e.g.BitbucketIncrementalStream.repository_is_idle(repo, prior) -> bool.src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/file_changes.py#L17-L24: replace with a call to the same shared helper.src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commit_branch_reachability.py#L18-L25: replace with a call to the same shared helper.src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/branches.py#L33-L36: replace with a call to the same shared helper (this site already differs slightly from the other three and would become consistent).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py` around lines 17 - 24, Move the shared repo_updated_on-versus-prior idle check into BitbucketIncrementalStream as repository_is_idle(repo, prior), preserving its existing comparison semantics. In src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py#17-24, file_changes.py#17-24, commit_branch_reachability.py#18-25, and branches.py#33-36, replace each duplicated guard with the shared helper and retain the existing early-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/metric_events.py`:
- Around line 124-139: Update PipelineStateStream.pipeline_candidates to include
the prior open-pipeline state in cache_key alongside repo_state_key(repo) and
created_on. Ensure equivalent open lists produce a stable key and different open
states cannot share cached selections, while preserving the existing cache
lookup, fetch, and state-return behavior.
---
Nitpick comments:
In
`@src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py`:
- Around line 17-24: Move the shared repo_updated_on-versus-prior idle check
into BitbucketIncrementalStream as repository_is_idle(repo, prior), preserving
its existing comparison semantics. In
src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.py#17-24,
file_changes.py#17-24, commit_branch_reachability.py#18-25, and
branches.py#33-36, replace each duplicated guard with the shared helper and
retain the existing early-return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 042adb8a-a169-45ee-9159-59b175680a70
📒 Files selected for processing (13)
src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/client.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/branches.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commit_branch_reachability.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/commits.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/file_changes.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/metric_events.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/pr_base.pysrc/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/streams/pull_requests.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/conftest.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_commits.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_reliability.pysrc/ingestion/connectors/git/bitbucket-cloud/tests/test_request_budget.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ingestion/connectors/git/bitbucket-cloud/tests/conftest.py
- src/ingestion/connectors/git/bitbucket-cloud/source_bitbucket_cloud/client.py
- src/ingestion/connectors/git/bitbucket-cloud/tests/test_inaccessible_repos.py
mitasovr
approved these changes
Jul 29, 2026
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
3 participants
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.
A repository can appear in the workspace listing and deny every request under it (403) — routine with repo-scoped tokens, common in production. The connector failed the whole sync over it, on every run, forever. A review for other members of the same class (foreseeable reality handled as a failure) found four more; all fixed here. A second review pass targeted the other production constraint — Bitbucket's ~1,000 req/h budget vs ~1,400 repositories — and cut the per-sync request count.
Fixes
diffstat/{sha}gone: orphaned merges, rewritten history) → tolerated; marker records the denial so the completeness gate keeps prior line counts instead of zeroing them. Previously one such commit failed its repository every sync. The pre-rewrite connector handled this (ignore_404); the rewrite lost it.bitbucket_username→ Bearer rejection, or rotated token) instead of 1,400 per-repo quarantine logs ending in a generic error.Rate-limit cuts
updated_oncomes free with the workspace listing and is stored in per-repo state; unchanged → branches, commits, file_changes and reachability skip the repository without a single request. The previous branch generation simply stays the newest complete one.Net: an idle repository costs ~5 requests per sync instead of ~13–25.
Not changed
Feature-level fetches (comments, diffstat-per-PR, activity, tasks, issues, pipelines) already tolerated 403 via
snapshot_available=false— that is what stops a denied read from being recorded as a legitimate empty collection and deleting rows. Untouched.Tests
Matrix derives the stream list from
source.streams()and runs every stream — current and future — against an all-403 repository (sync green, no items, markers unavailable, state frozen) and a hard 500 (still fails loudly). Each fix has a test that fails without it. Per-repo branch gating verified with data on ClickHouse 25.7.5: newer generation replaces, denied repo retains, no coupling. Request budget pinned by counting clients: idle repo → zero calls, changedupdated_on→ resync, migrated legacy state never gated, children reuse the parent's listing and emit records identical to a fresh fetch, slim cache rejects raw objects. 186 pass.Summary by CodeRabbit
Bug Fixes
Performance