fix(worker): extend permission-sync fail-closed to HTTP 410#1216
Conversation
PR #1215 added a fail-closed branch in the account-driven permission syncer that clears an account's AccountToRepoPermission rows when the upstream call throws 401, 403, or a token-refresh error. The predicate set is too narrow for endpoint deprecations: Bitbucket Cloud's CHANGE-2770 removed GET /2.0/user/permissions/repositories and the endpoint now returns HTTP 410 Gone for every caller. With the existing predicate, the syncer aborts without clearing the account's rows, so stale permissions persist indefinitely. Add `isGone` (status 410) alongside `isUnauthorized` (401) and `isForbidden` (403), and include it in the catch in accountPermissionSyncer.runJob. The cleanup is the same scorched-earth deleteMany used by the existing predicates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds an exported ChangesAccount permission syncer 410 Gone handling
Sequence Diagram(s)sequenceDiagram
participant AccountPermissionSyncer
participant UpstreamAPI
participant errors_isGone
participant accountToRepoPermission_DB
AccountPermissionSyncer->>UpstreamAPI: fetch permissions request
UpstreamAPI-->>AccountPermissionSyncer: error response (HTTP 410)
AccountPermissionSyncer->>errors_isGone: isGone(error)
errors_isGone-->>AccountPermissionSyncer: true
AccountPermissionSyncer->>accountToRepoPermission_DB: DELETE WHERE accountId = X
accountToRepoPermission_DB-->>AccountPermissionSyncer: delete result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the account-driven permission syncer's catch fires (401/403/410/ token-refresh), it silently wipes the account's AccountToRepoPermission rows before re-throwing. Operators can see the upstream error in three sinks already, but there's no signal that the *cleanup* itself ran — forcing a before/after row count to verify behavior. Switch the cleanup to a direct `accountToRepoPermission.deleteMany` so we get the deleted row count, and emit a warn log with the account id, user email, which predicate matched, the count, and the original error message. Behavior is equivalent to the prior `account.update` form (same DELETE WHERE accountId=...). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 209-210: Remove PII and raw upstream error text from the
fail-closed warning: update the logger.warn call in accountPermissionSyncer (the
statement that currently references logger.warn and interpolates
account.user.email and the error.message/String(error)) so it only logs
account.id, reason and count (and any fixed context text), and do not include
account.user.email or the raw error message; rely on Sentry/failed-job records
for full error details instead.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e56ba7a2-590e-4a3d-8129-3747112696c2
📒 Files selected for processing (1)
packages/backend/src/ee/accountPermissionSyncer.ts
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #1215 added a fail-closed branch in the account-driven permission syncer (
accountPermissionSyncer.runJob) that clears an account'sAccountToRepoPermissionrows when the upstream call throws 401, 403, or a token-refresh error. The predicate set is too narrow for endpoint deprecations:Bitbucket Cloud's CHANGE-2770 removed
GET /2.0/user/permissions/repositoriesand the endpoint now returns HTTP 410 Gone for every caller. With the existing predicate, the syncer aborts without clearing the account's rows, so stale permissions persist indefinitely. This PR makes 410 errors trigger that clean up logicSummary by CodeRabbit
Bug Fixes
Tests